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

meta: replace build-windows for test-windows #50519

Closed
wants to merge 2 commits into from

Conversation

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/actions

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Nov 2, 2023
@H4ad H4ad requested a review from richardlau November 2, 2023 01:31
@targos
Copy link
Member

targos commented Nov 2, 2023

 failed 10 out of 10
not ok 394 parallel/test-child-process-exec-any-shells-windows
  ---
  duration_ms: 662.63700
  severity: fail
  exitcode: 1
  stack: |-
    node:assert:991
        throw newErr;
        ^
    
    AssertionError [ERR_ASSERTION]: ifError got unwanted exception: Command failed: echo foo bar
    
        at D:\a\node\node\test\common\index.js:439:12
        at D:\a\node\node\test\common\index.js:476:15
        at ChildProcess.exithandler (node:child_process:430:5)
        at ChildProcess.exithandler (node:child_process:422:12)
        at ChildProcess.emit (node:events:519:28)
        at maybeClose (node:internal/child_process:1105:16)
        at ChildProcess._handle.onexit (node:internal/child_process:305:5) {
      generatedMessage: false,
      code: 'ERR_ASSERTION',
      actual: Error: Command failed: echo foo bar
      
          at ChildProcess.exithandler (node:child_process:422:12)
          at ChildProcess.emit (node:events:519:28)
          at maybeClose (node:internal/child_process:1105:16)
          at ChildProcess._handle.onexit (node:internal/child_process:305:5) {
        code: 1,
        killed: false,
        signal: null,
        cmd: 'echo foo bar'
      },
      expected: null,
      operator: 'ifError'
    }
    
    Node.js v22.0.0-pre
  ...

@H4ad
Copy link
Member Author

H4ad commented Nov 3, 2023

@targos This test is probably failing because of this error:

C:\Users\vinic\Desktop\Projects\node>"C:\Program Files\Git\usr\bin\bash.exe" ls
/usr/bin/ls: /usr/bin/ls: cannot execute binary file

But I didn't find a way to get that error message, at least when I print the error returned by exec, is just:

Error: Command failed: echo foo bar

    at ChildProcess.exithandler (node:child_process:422:12)
    at ChildProcess.emit (node:events:519:28)
    at maybeClose (node:internal/child_process:1105:16)
    at ChildProcess._handle.onexit (node:internal/child_process:305:5) {
  code: 1,
  killed: false,
  signal: null,
  cmd: 'echo foo bar'
}

If I try to inspect the message:

O Subsistema do Windows para Linux n�o tem distribui��es instaladas.
As distribui��es podem ser instaladas visitando a Microsoft Store:
https://aka.ms/wslstore

Is just a generic error in Portuguese about WSL, so I don't think searching and testing for bash is useful, maybe we can remove that part of the test.


By the way, the CI is green because I stopped calling the test-child-process-exec-any-shells-windows.js, from what I look on other CIs in Windows, they don't call that test (I put the link on description).

@H4ad H4ad requested a review from targos November 3, 2023 02:01
@H4ad
Copy link
Member Author

H4ad commented Nov 4, 2023

@targos @richardlau

Is it okay to remove a part of the test?

Based on #50519 (comment), I don't think that test will run on Github CI, that test doesn't even run on my machine.

Basically, we just need to remove the part that is testing bash on WSL.

@targos
Copy link
Member

targos commented Nov 4, 2023

Is it okay to remove a part of the test?

No. We need to install Git bash. It's a requirement for running the tests: https://github.com/nodejs/node/blob/main/BUILDING.md#option-1-manual-install

@H4ad
Copy link
Member Author

H4ad commented Nov 4, 2023

@targos Bash is already installed: https://github.com/nodejs/node/actions/runs/6751686476/job/18356112178?pr=50519#step:4:50

As I mentioned on #50519 (comment), I don't think the test will be able to run when Git Bash is installed or when WSL is enabled but not configured.

I suspect the environment we run the tests doesn't have the Git Bash, or at least, doesn't have the first one:

$ where bash <-- running on my machine
C:\Program Files\Git\usr\bin\bash.exe <-- this one that causes issues on the test because of WSL
C:\Windows\System32\bash.exe

@H4ad H4ad force-pushed the ci/add-win-test branch 4 times, most recently from 85b0a66 to 346cc37 Compare November 5, 2023 20:19
@aduh95
Copy link
Contributor

aduh95 commented May 11, 2024

This needs a rebase.

Comment on lines +56 to +62
const skipPaths = [
// Skip bash from WSL because of
// https://github.com/nodejs/node/pull/50519#issuecomment-1791806986
'usr\\bin\\bash.exe',
];

// Test Bash (Git), if available
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only part of the problem, it keeps failing when testing cmd

@H4ad
Copy link
Member Author

H4ad commented Jul 20, 2024

Closing this one since I have no clue on how I should properly fix those issues on Win :/

@H4ad H4ad closed this Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a GHA Windows test workflow?
7 participants