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

One unit test fails with pnpm, works with npm #7368

Closed
rmunn opened this issue Mar 15, 2022 · 11 comments
Closed

One unit test fails with pnpm, works with npm #7368

rmunn opened this issue Mar 15, 2022 · 11 comments

Comments

@rmunn
Copy link
Contributor

rmunn commented Mar 15, 2022

Describe the bug

The test/sourcemaps/samples/compile-option-dev/test.js unit test somehow produces different results when you've installed Svelte dependencies with pnpm vs with npm. Since I prefer to use pnpm (which can deduplicate packages across node_modules folders from multiple projects, saving a lot of disk space), this means that every time I want to create a Svelte PR, I have to edit that test file and change the expected values in it, then make sure those edits don't make it into my PR.

Reproduction

  1. Rename test/sourcemaps/samples/compile-option-dev/ to test/sourcemaps/samples/compile-option-dev.solo/ to run only this test. (This doesn't change the results, just the speed of running the tests)
  2. Run npm i, then npm run test.
  3. Test passes.
  4. Run pnpm import to convert package-lock.json to a pnpm-lock.yaml file with the same package version numbers in it.
  5. Run rm -rf node_modules, then pnpm i to ensure that PNPM is the one that has created the node_modules folder
  6. Run pnpm run test (or npm run test, both will produce the same result)
  7. Test fails: see below.

The test failure is caused by the column numbers produced by SourceMapConsumer.originalPositionFor from the source-map package. When that package is installed via npm, the two column numbers checked for in that test are 5. When the package is installed via pnpm, the two column numbers are 4 instead of 5. (The logs only show one test failure, on the --done-replace-once check, but if you edit its column number to expect 4, the --done-replace-twice check will also fail with actual=4 and expected=5). Logs from a failed test run (running pnpm run test) are below.

Logs

sourcemaps
    1) compile-option-dev.solo


  0 passing (66ms)
  1 failing

  1) sourcemaps
       compile-option-dev.solo:

      AssertionError [ERR_ASSERTION]: failed to locate "--done-replace-once" from "input.svelte"
      + expected - actual

       {
      -  "column": 4
      +  "column": 5
         "line": 6
         "name": [null]
         "source": "input.svelte"
       }
      
      at /.../svelte/test/sourcemaps/samples/compile-option-dev.solo/test.js:27:10
      at Array.forEach (<anonymous>)
      at test (test/sourcemaps/samples/compile-option-dev.solo/test.js:26:3)
      at Context.<anonymous> (test/sourcemaps/index.ts:117:4)



 ELIFECYCLE  Test failed. See above for more details.

System Info

System:
    OS: Linux 5.4 Linux Mint 19.1 (Tessa)
    CPU: (12) x64 Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
    Memory: 8.33 GB / 62.58 GB
    Container: Yes
    Shell: 4.4.20 - /bin/bash
  Binaries:
    Node: 16.14.0 - /usr/local/bin/node
    Yarn: 1.22.17 - /usr/bin/yarn
    npm: 7.23.0 - ~/.local/bin/npm
  Browsers:
    Chrome: 74.0.3729.169
    Firefox: 96.0.3
  npmPackages:
    rollup: ^1.27.14 => 1.32.1

Severity

annoyance

@rmunn
Copy link
Contributor Author

rmunn commented Mar 15, 2022

The failing test was added in #5584 and has not been touched since. My proposed fix would be to edit the test to allow the test to pass when column value is either 4 or 5, since both of those values are correct under some configuration but values that are neither 4 nor 5 would indicate a bug of some kind.

The root cause of this issue, of course, is in either pnpm or the source-map package, but I don't have time right now to go digging into those projects' source to figure out why source-map is acting slightly differently when installed via pnpm vs. npm. So I'm proposing a small tweak to this test in order to patch over the difference and allow the unit tests to pass whether pnpm or npm is used to install them.

Of course, it's also possible that this is something unique to my system. If nobody else can reproduce the issue by using pnpm, then I'll close the issue and figure out what's different about my setup. But if this is an issue that affects anyone who uses pnpm in the Svelte repo, then it's probably worth applying a band-aid over this one test to allow the column value to vary by 1.

@Prinzhorn
Copy link
Contributor

Can you post or diff both npm ls --all? Just to make sure step 4 and 5 actually does what we think it does.

@rmunn
Copy link
Contributor Author

rmunn commented Mar 16, 2022

Here are the two lists, one with npm i and the other after pnpm import; pnpm i. Unfortunately, the graph characters make this difficult to diff, because pnpm nests the visual graph slightly differently than npm does and the result is a diff that shows every line changed. I'll try using the --parseable and --json options to see if I can get a format that will be properly diffable. But a visual comparison between these two lists should be possible, if tedious; if I can't get a properly diffable result then these files will have to do.

npm-list.txt
pnpm-list.txt

P.S. The second list was created with pnpm list --depth 99 because npm doesn't understand pnpm's node_modules format, and running npm ls --all after pnpm i resulted in npm complaining that every single dependency was missing. :-) So I had to use npm ls --all to produce the first list, and pnpm list --depth 99 to produce the second one, hence why their outputs are in slightly different formats and are hard to diff.

@rmunn
Copy link
Contributor Author

rmunn commented Mar 16, 2022

Here are the two lists with --json. Again, not directly diffable, but this time it looks like I'll be able to massage them into being similar enough to diff, by removing the "devDependencies" and "unsavedDependencies" properties from the pnpm list and moving all their children up by one. That should line the two files up to be diffable. In case I can't get to it promptly, though, here are the untouched .json files in case they're helpful:

npm-list.json.txt
pnpm-list.json.txt

Note: renamed .json to .json.txt so that GitHub would allow uploading the files as issue attachments (it was rejecting the .json extension).

@rmunn
Copy link
Contributor Author

rmunn commented Mar 16, 2022

Nope, still not diffable as pnpm has way more details in its json output than npm does. But I did do a Ctrl+F for source-map and verified that the same version was installed by npm and pnpm: they were both grabbing source-map 0.7.3, and also listed source-map as a dependency of css-tree with source-map version 0.6.1 appearing in the css-tree dependency list. Same versions for both npm and pnpm, so I haven't found the difference yet.

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Mar 16, 2022

Unfortunately, the graph characters make this difficult to diff, because pnpm nests the visual graph slightly differently than npm does and the result is a diff that shows every line changed

A regex replace for ^[├─└┬]+ plus sort and uniq (Sublime Text has both also integrated) goes a long way to make things more diffable. But I didn't see any useful differences 🤷 . I tried to repro this in a Docker container, but pnpm didn't like that so I don't think I can be of any more assistance here.

@rmunn
Copy link
Contributor Author

rmunn commented May 29, 2023

Can no longer reproduce, and development is moving on to version 4 so any test issues on the master branch are no longer really relevant anyway. Closing.

@rmunn rmunn closed this as completed May 29, 2023
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

No branches or pull requests

3 participants
@rmunn @Prinzhorn and others