Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: add c++ test coverage #1193

Closed
bnoordhuis opened this issue Mar 18, 2015 · 10 comments
Closed

src: add c++ test coverage #1193

bnoordhuis opened this issue Mar 18, 2015 · 10 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. test Issues and PRs related to the tests.

Comments

@bnoordhuis
Copy link
Member

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.

@indutny
Copy link
Member

indutny commented Mar 18, 2015

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.

@indutny
Copy link
Member

indutny commented Mar 18, 2015

+1 for this in general, though

@bnoordhuis
Copy link
Member Author

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. :-)

@indutny
Copy link
Member

indutny commented Mar 18, 2015

Sounds awesome! Go ahead with it :)

@jbergstroem
Copy link
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?

@bnoordhuis
Copy link
Member Author

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.

@mscdex mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. test Issues and PRs related to the tests. labels Mar 19, 2015
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Mar 19, 2015
Check in a gypified gtest and add a simple unit test to show that the
basic infrastructure is in place.

Refs: nodejs#1193
@trevnorris
Copy link
Contributor

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

@bnoordhuis
Copy link
Member Author

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

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Apr 1, 2015
Check in a gypified gtest and add a simple unit test to show that the
basic infrastructure is in place.

PR-URL: nodejs#1199
Refs: nodejs#1193
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@Fishrock123
Copy link
Contributor

Can this be closed now? :)

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

@bnoordhuis
Copy link
Member Author

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
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

6 participants