Skip to content

src: add c++ test coverage #1193

Description

@bnoordhuis

Meta-issue for something that was discussed in today's TC meeting: test coverage for C++ code that is only tested indirectly now.

Strawman proposal: bundle gtest, make it part of make test and start churning out src/*_unittest.cc tests like chromium has.

Problem: most of the code in src/ is not very modular right now. Turning it into something that is easily testable will no doubt create a lot of churn. Probably unavoidable but it's acceptable to me.

/cc @indutny @piscisaureus @trevnorris - if we can get some consensus on this, I volunteer to do the initial work.

Activity

  1. indutny commented on Mar 18, 2015

    @indutny
    Member

    It is very interesting thing. What I am mostly curious about is - how much of the stuff will we be able to test within the C++? It seems to me that many if not most of the code is tightly bound to the JS.

    The second question is even more interesting :) Do we want it to be cross-platform? Or can we do some raw socket() machinery in it? Like working with fds, and stuff like that.

  2. indutny commented on Mar 18, 2015

    @indutny
    Member

    +1 for this in general, though

  3. bnoordhuis commented on Mar 18, 2015

    @bnoordhuis
    MemberAuthor

    It is very interesting thing. What I am mostly curious about is - how much of the stuff will we be able to test within the C++? It seems to me that many if not most of the code is tightly bound to the JS.

    That's why I predict there will be a fair bit of churn, to break out the meat from the glue code. I think it's alright when tests set up a VM where it makes sense.

    The second question is even more interesting :) Do we want it to be cross-platform? Or can we do some raw socket() machinery in it? Like working with fds, and stuff like that.

    It should be cross-platform, with the platform-dependent bits guarded by #ifdef statements. Just like libuv, really. :-)

  4. indutny commented on Mar 18, 2015

    @indutny
    Member

    Sounds awesome! Go ahead with it :)

  5. jbergstroem commented on Mar 18, 2015

    @jbergstroem
    Member

    Good idea indeed. Is there perhaps a .gyp for gtest somewhere? I'd really despise having to pull cmake or auto* into the dependency chain. Another plan could be making running these tests optional and assume you can handle installing gtest yourself?

  6. bnoordhuis commented on Mar 19, 2015

    @bnoordhuis
    MemberAuthor

    I don't think there is one currently but it should be pretty easy to get it gypified. gtest core is only a handful of files.

  7. added
    c++Issues and PRs that require attention from people who are familiar with C++.
    testIssues and PRs related to Node.js core tests and test infrastructure.
    on Mar 19, 2015
  8. trevnorris commented on Mar 19, 2015

    @trevnorris
    Contributor

    I'm fine with this. Will it mean make test-addon will be removed/unneeded?

  9. bnoordhuis commented on Mar 19, 2015

    @bnoordhuis
    MemberAuthor

    Not right away but it could, longer term. Not the part of make test-addon that scrapes the API documentation, though.

  10. Fishrock123 commented on Apr 11, 2015

    @Fishrock123
    Contributor

    Can this be closed now? :)

    Edit: Assuming it's open until we add more coverage?

  11. bnoordhuis commented on Apr 11, 2015

    @bnoordhuis
    MemberAuthor

    Yes, it's fixed (if you can call it that) in 0080788. Adding tests for code in src/ is an ongoing process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    c++Issues and PRs that require attention from people who are familiar with C++.testIssues and PRs related to Node.js core tests and test infrastructure.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions