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

feature: add way of testing deps of a module #765

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

andrewhughes101
Copy link
Contributor

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 this

Checklist
  • npm test passes
  • tests are included
  • documentation is changed or added
  • contribution guidelines followed
    here

@andrewhughes101
Copy link
Contributor Author

Refs #631 could be used to automate this

Copy link
Member

@richardlau richardlau left a 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.

bin/citgm-deps.js Outdated Show resolved Hide resolved
bin/citgm-deps.js Outdated Show resolved Hide resolved
bin/citgm-deps.js Outdated Show resolved Hide resolved
bin/citgm-deps.js Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Nov 1, 2019

Codecov Report

Merging #765 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c3967d...bd38d3f. Read the comment docs.

@andrewhughes101
Copy link
Contributor Author

@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 citgm-deps would do is the npm install and grab the versions of each dependency.

@nodejs/citgm opinions on adding an optional version field to the lookup?

@BethGriggs
Copy link
Member

ping @nodejs/citgm on behalf of @andrewhughes101

@MylesBorins
Copy link
Member

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 tags to similarly filter subsets of modules if we wanted to only test supported modules. A script that automated updating the lookup.json with tags that grouped related modules would likely be very helpful for that.

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

@BridgeAR
Copy link
Member

BridgeAR commented Nov 6, 2019

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.

@mhdawson
Copy link
Member

mhdawson commented Nov 6, 2019

@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?

@andrewhughes101
Copy link
Contributor Author

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?

@MylesBorins
Copy link
Member

/cc @ljharb who was just talking to me about smoketesting needs as a module author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants