lirantal / lockfile-lint

Lint an npm or yarn lockfile to analyze and detect security issues

Apache License 2.0

785 stars 35 forks source link

fix(lockfile-lint): fixed failing tests depending on user locale #50

Closed MathieuAA closed 4 years ago

MathieuAA commented 4 years ago

Description

Made the tests not fail if the user locale isn't the default one (en). This issue occurred because yargs detects the locale to output things.

Types of changes

Disabled the locale detection when configuring yargs.

[X] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Related Issue

Fix #41

Motivation and Context

I wanted to play with this great package but the tests didn't pass.

How Has This Been Tested?

I cloned the project, installed the packages' dependencies and launched the tests. They failed. I did the fix and checked the tests passed.

Screenshots (if appropriate):

Checklist:

[x] I have updated the documentation (if required).
[x] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes
[X] All new and existing tests passed.
[X] I added a picture of a cute animal cause it's fun

codecov-io commented 4 years ago

Codecov Report

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

@@           Coverage Diff           @@
##           master      #50   +/-   ##
=======================================
  Coverage   97.88%   97.88%           
=======================================
  Files          11       11           
  Lines         189      189           
  Branches       29       29           
=======================================
  Hits          185      185           
  Misses          4        4

Impacted Files	Coverage Δ
packages/lockfile-lint/src/cli.js	`0% <ø> (ø)`	:arrow_up:

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 c7671ac...8a6e1ad. Read the comment docs.

lirantal commented 4 years ago

Looks good, thanks @MathieuAA 💜

lirantal commented 4 years ago

I'll follow-up this PR with a travis update that also tests for other locales

MathieuAA commented 4 years ago

I don't think it's a good idea. Checking whether some word appears in an output controlled by a dependency is good if you're testing the dependency's integration with your tool. If the word changes, or a typo is introduced, your tests will fail. The coupling is too strong.

lirantal commented 4 years ago

@MathieuAA I don't fancy the change introduced in this PR to disable automatic locale detection just to fix the developer-side failing test but I'm ok with it as a workaround since localization of the CLI text isn't a feature I expect out of this tool.

The actual fix should be that when the tests run they invoke the CLI in a way that doesn't auto-detect the locale.

Testing based on CLI output makes sense because I do want to ensure that the CLI has helplful information like showing an example to the user.

MathieuAA commented 4 years ago

If it's actually something that matters, then yes: disabling locale detection for tests may be the way to go.

In this case, instead of checking if the "examples" word is present (yargs introduces this word), might I suggest that what you wrote as text be checked directly? That way you're in control of what you provide to the user and what's displayed by yargs and don't depend on what the dependency may or may not provide as extra chars. And if you really want to check an "examples" string be present, you can always write dependency tests separately.