-
Notifications
You must be signed in to change notification settings - Fork 150
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
feature: add way of testing deps of a module #765
Conversation
Refs #631 could be used to automate this |
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.
Test (lint) failures need to be addressed. Needs tests. I'm +0 on whether this should be an option to either citgm
or citgm-all
instead of a new command in its own right.
Codecov Report
@@ Coverage Diff @@
## master #765 +/- ##
======================================
Coverage 95.5% 95.5%
======================================
Files 27 27
Lines 889 889
======================================
Hits 849 849
Misses 40 40 Continue to review full report at Codecov.
|
@richardlau I think an alternative way of producing this functionality would be to add an optional version field to the lookup for each module that citgm-all can handle. Then all that @nodejs/citgm opinions on adding an optional version field to the lookup? |
ping @nodejs/citgm on behalf of @andrewhughes101 |
I'm not 100% that this is the best way to approach the problem outlined in #631 Generally packages don't "just work" with CITGM. Usually we have to manually add them to the lookup table including meta data such as version prefix, package manager default, and what platforms that module is flaky on. If we automate the process of testing all of the dependencies installed I fear we will end up with a ton of false negatives and in turn have to manually add all of those dependencies into the lookup table before we can get accurate results. At that point a CITGM-ALL run will cover all those dependencies. We could likely use I'm not going to block this, but would like to hear a bit more about how this will be used and how the data we get from the script will be useful before supporting this feature |
I agree with @MylesBorins that this might end up with a lot of flaky tests in the test suite. It might be a good idea to try to automate a few more things that we require less meta data (e.g., the version prefix and the default package manager could be detected). What platforms a module is flaky on is something difficult to automate though. I think this is a good idea on a longer term. I am just not sure if we are already at the point that this works as expected. |
@BridgeAR, @MylesBorins when this work was mentioned to me it was in the context of helping projects run test themselves (for example the Express project) which I think would be useful and a good extension of CITGM. I'm thinking we should add the functionality to do that in a way that does not affect our existing CITGM runs (ie we won't need to worry about any flakiness) but does let package maintainers experiment with using CITGM to do additional testing. What do you think? |
The plan was to implement this as a new command and not add it to the CITGM runs. This is so it could be used by module maintainers to test their module dependencies using CITGM without affecting existing nodejs CI test runs. If there are features that module maintainers could use in CITGM, then visibility of CITGM could be increased, potentially leading to more contributors? |
/cc @ljharb who was just talking to me about smoketesting needs as a module author |
New feature to test all the dependencies of a module. This will test the versions of the dependencies that get installed at the time of
npm install
. Current work in progress tests and documentation to come. Please let me know if this should be intergarted into citgm or citgm-all - help would be appreciated on how to do thisChecklist
npm test
passeshere