8000 [Local Dev][Bug] - @oclif/core tests are not hermetic · Issue #1364 · oclif/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Local Dev][Bug] - @oclif/core tests are not hermetic #1364

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.

Sign up for GitHub

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

Jump to bottom
Open
spanishpear opened this issue Apr 26, 2025 · 5 comments
Open

[Local Dev][Bug] - @oclif/core tests are not hermetic #1364

spanishpear opened this issue Apr 26, 2025 · 5 comments
Labels
bug Something isn't working help wanted Accepting PRs

Comments

@spanishpear
Copy link
Contributor
spanishpear commented Apr 26, 2025 •
edited
Loading

Hi folks!

Context

I was writing #1363 and when running the local devloop outlined in CONTRIBUTING.md - the tests as per

yarn test

failed, as the oclif --version command was returning a different architechture

-  @oclif/core/4.2.10 wsl-x64 node-v20.19.1
+  @oclif/core/4.2.10 linux-x64 node-v20.19.1

Similarly the darwin tests fail, because, well - I'm not on darwin ;)

-/my/home/.cache/@oclif/core
+/my/home/Library/Caches/@oclif/core

Report

this resulted in 11 failing tests locally:

  • main-esm > runs --version
  • main-esm > runs --help
  • main > should run version
  • main > should run help
  • Config > Darwin > should have darwin specific paths
  • Config > win32 > should have win32 specific paths
  • formatRoot > renders the root help
  • formatRoot > description > splits on > for the description in the top level and description sections
  • formatRoot > description > shows description from a template
  • formatRoot > description > prefers the oclif description over the package.json description
  • formatRoot > description > uses package.json description when the oclif description is not set

when running on WSL!
@spanishpear spanishpear changed the title [Local Dev] @oclif/core tests are not hermetic [Local Dev][Bug] - @oclif/core tests are not hermetic Apr 26, 2025
@cristiand391 cristiand391 added the bug Something isn't working label Apr 29, 2025
@git2gus Git2Gus
Copy link
git2gus bot commented Apr 29, 2025

This issue has been linked to a new work item: W-18398048

@mdonnalley mdonnalley added the help wanted Accepting PRs label Jun 2, 2025
@mdonnalley
Copy link
Contributor
mdonnalley commented Jun 2, 2025 •
edited
Loading

@spanishpear Are you still seeing this? I don't have a windows machine to test on but the tests are passing for me on both mac and linux

Also worth mentioning that our unit tests run in CI against windows, mac, and linux - and they all pass. Although it could be that you're using WSL now that I think about it

If it's a WSL specific issue, I won't be able to find the time to fix this. Especially since we don't officially support WSL. However, I'd be happy to review and merge a PR if you are interested in submitting one

@spanishpear
Copy link
Contributor Author

@mdonnalley Hey! Yeah, I just rebased and get the same failures

  10) formatRoot
       description
         prefers the oclif description over the package.json description:

      AssertionError: expected 'THIS IS THE OCLIF DESCRIPTION IN PJSO…' to equal 'THIS IS THE OCLIF DESCRIPTION IN PJSO…'
      + expected - actual

       THIS IS THE OCLIF DESCRIPTION IN PJSON

       VERSION
      -  @oclif/core/4.2.10 wsl-x64 node-v23.5.0
      +  @oclif/core/4.2.10 linux-x64 node-v23.5.0

       USAGE
         $ oclif [COMMAND]

      at Context.<anonymous> (test/help/format-root.test.ts:92:25)
      at processImmediate (node:internal/timers:511:21)

So it might indeed be something WSL specific ⁉
Will try and poke at it a bit more when I get some free time ❤

@spanishpear
Copy link
Contributor Author
spanishpear commented Jun 8, 2025 •
edited
Loading

Okay, looks like the tests are actually failing as expected, because the tests are wrong 😅

There is code that explicitly sets this.platform = 'wsl', if it's running in WSL. This explains why the test failure doesn't reproduce on anything but WSL.

core/src/config/config.ts

Line 332 in 42f37b6

this.platform = WSL ? 'wsl' : getPlatform()

However the "expected" output for the test is setup to use process.platform instead, which correctly uses linux.

core/test/command/main.test.ts

Line 7 in 42f37b6

const version = `@oclif/core/${pjson.version} ${process.platform}-${process.arch} node-${process.version}`

As such we end with the following failure.

-@oclif/core/4.3.2 wsl-x64 node-v22.16.0
+@oclif/core/4.3.2 linux-x64 node-v22.16.0

So, the test fails... but because the expected output is incorrect, and not testing reality!

To make matters even more interesting, there are other failing tests that stub getPlatform

core/test/config/config.test.ts

Line 251 in 42f37b6

sinon.stub(os, 'getPlatform').returns('darwin')

but because the implementation is effectively

core/src/config/config.ts

Line 332 in 42f37b6

this.platform = WSL ? 'wsl' : getPlatform()

stubbing getPlatform is meaningless, as it doesn't even get evaluated 🤦‍♀

Tracing back, it seems it was added in oclif/config 1.16
as an intentionally distinguishable option from linux - oclif/config#106

There are only two usages of wsl as a identifier - so it's for third party consumers.
https://github.com/search?q=repo%3Aoclif%2Fcore%20%27wsl%27&type=code

Potential avenues forward...

  • move wsl check into getPlatform, and mock accordingly (preferred)
  • update the tests only to change their expected output to correctly test for wsl, keep implementation as is
  • remove WSL support entirely (will regress WSL for config.platform config#106 🤷‍♀

I will probably submit a PR for the first option if you're happy with that direction @mdonnalley ?

@mdonnalley
Copy link
Contributor

@spanishpear thanks for writeup! The first option sounds good to me. I'm also happy with the second option if the first doesn't work out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working help wanted Accepting PRs
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@cristiand391 @mdonnalley @spanishpear
0