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

lookup: re-add npm #979

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

lookup: re-add npm #979

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 24, 2023

Checklist
  • npm test passes
  • contribution guidelines followed
    here

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2023

Codecov Report

Patch coverage has no change and project coverage change: -1.13% ⚠️

Comparison is base (f15c9a2) 96.44% compared to head (98eccdf) 95.32%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #979      +/-   ##
==========================================
- Coverage   96.44%   95.32%   -1.13%     
==========================================
  Files          28       28              
  Lines        2139     2139              
==========================================
- Hits         2063     2039      -24     
- Misses         76      100      +24     

see 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Trott
Copy link
Member Author

Trott commented Sep 24, 2023

@Trott
Copy link
Member Author

Trott commented Sep 24, 2023

The tests that set LC_ALL to 'sk' all fail because (I think) that locale is not on our Jenkins servers.

 sh: warning: setlocale: LC_ALL: cannot change locale (sk): No such file or directory
 sh: warning: setlocale: LC_ALL: cannot change locale (sk): No such file or directory
 sh: warning: setlocale: LC_ALL: cannot change locale (sk): No such file or directory
 sh: warning: setlocale: LC_ALL: cannot change locale (sk): No such file or directory
 sh: warning: setlocale: LC_ALL: cannot change locale (sk): No such file or directory
 sh: warning: setlocale: LC_ALL: cannot change locale (sk): No such file or directory
 sh: warning: setlocale: LC_ALL: cannot change locale (sk): No such file or directory

@nodejs/npm Is there an easy way to skip those tests or somehow accommodate another locale?

@nodejs/build Is there an easy way for us to add that locale?

@lukekarrys
Copy link
Member

Originally this locale was used in the test environment to ensure the correct sorting behavior when no locale was set in the CLI code: npm/cli@1d09214.

It's possible that the tests will still pass without it and we can remove it for citgm but keep it in npm/cli to stop any regressions. I will confirm whether this is true or not.

If it is, then there is a package.json config that could be unset before the tests run. I regret that I haven't looked closer yet into npm's failures in citgm, so I'm not sure if this is possible. Is it possible to run a setup command prior to the tests in citgm only? npm pkg delete tap.test-env should do the trick.

@lukekarrys lukekarrys self-assigned this Sep 25, 2023
@Trott
Copy link
Member Author

Trott commented Sep 25, 2023

I regret that I haven't looked closer yet into npm's failures in citgm, so I'm not sure if this is possible.

I'm not sure, but it might have only just started happening. We've made more than few changes to CITGM.

Is it possible to run a setup command prior to the tests in citgm only? npm pkg delete tap.test-env should do the trick.

I think we'd just need to put it into the "scripts" setting for npm in the CITGM package.json. EDIT: I thought it was already built into CITGM to do that sort of thing, but I think I was wrong.

@Trott
Copy link
Member Author

Trott commented Sep 25, 2023

For comparison, running CITGM on npm using CITGM@8. If this passes, then we'll bisect and figure out what changed in CITGM to cause the failures: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3262/

@Trott
Copy link
Member Author

Trott commented Sep 25, 2023

For comparison, running CITGM on npm using CITGM@8. If this passes, then we'll bisect and figure out what changed in CITGM to cause the failures: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3262/

CITGM@8 fails too, so the problem isn't something that CITGM@9 newly introduces.

@richardlau
Copy link
Member

@Trott FYI For a long time npm has been failing in citgm #897

@Trott Trott marked this pull request as draft September 26, 2023 08:53
@ruyadorno
Copy link
Member

Is it possible to run a setup command prior to the tests in citgm only? npm pkg delete tap.test-env should do the trick.

@lukekarrys in order to tweak a value for the generic citgm run, it looks like there are a few possibilities, looking at the current lib/lookup.json I see there are ways to define either a custom install command, or a scripts that can run or a envVar to set an environment variable. Hopefully using one of these can help you get tests passing again on citgm so that we can readd npm.

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.

5 participants