-
Notifications
You must be signed in to change notification settings - Fork 409
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
test(binaryinstall): Add initial tests #517
Conversation
76b4883
to
0c98778
Compare
Some things that I would especially appreciate comments on, are about spinning up a local server for testing the downloading of tarballs. I think it's a good idea to spin up a local server rather than hitting a server somewhere else but I'm not sure about the ports of the server. Since the tests are run in parallel, each test that want to spin up a server needs its own port to not get into race condition and error with "port already used". This is somewhat error-prone and not ideal. What are your thoughts about this? Another thing I'm thinking about is the naming of the tests. Should the function under test be mentioned in the test name? As I have done now it doesn't but maybe that's somewhat weird since all the functions are in the same file? |
2c263e4
to
f6c2464
Compare
@drager Existing tests already potentially download quite a lot from external servers -- including That said, personally I believe that spinning up the local server is quite clever and would save on bandwidth. (And it even works on my Windows machine!) Regarding the concern with accessing the server from multiple parallel tests, you may want to look at how Finally, I also noticed you have some tests directly in |
@danwilhelm Thanks for your comments! Yeah, existing tests in wasm-pack hits external servers, however binary-install will probably be put in it's own repo in the future. I mean, it's not absolutely necessary for binary-install to make the same decisions as wasm-pack but yeah, hitting external servers might not be a large concern, though I like the idea of be able to work fully offline. Thanks for the suggestion on using lock(), will check that out! About the tests in lib, my thoughts was that I would put tests in there rather than making functions public that wasn't meant to be public. |
f6c2464
to
e4fdfd7
Compare
I think it would be great to get your thoughts/review of this @ashleygwilliams, @alexcrichton or @fitzgen. 😊 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I made a few comments inline, but I think this would be good to merge after those things are addressed.
Thanks @drager!
test: Put tests that access private functions in tests module
@fitzgen Thanks for the review! I have now addressed all the comments. Let me know if this is good to go and I can squash all the commits into one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this is really great! thank you so much for your hard work on this!
This PR will add some tests to the binary install crate. Please review even that it's WIP. I appreciate any comments what so ever.