Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

test: use fs.execFile instead of exec #2110

Merged
merged 3 commits into from
Dec 23, 2024
Merged

Conversation

targos
Copy link
Contributor

@targos targos commented Dec 23, 2024

Generally safer than building a shell script, and allows to run the
tests from a directory with spaces in it.

Generally safer than building a shell script, and allows to run the
tests from a directory with spaces in it.
@targos
Copy link
Contributor Author

targos commented Dec 23, 2024

Example failure
​ FAIL ​ test/pkg/pkg.test.js
 ✖Command failed: npx pkg /Users/mzasso/git/forks/pino with spaces/test/pkg/index.js --config /Users/mzasso/git/forks/pino
  with spaces/test/pkg/pkg.config.json (node:8620) [DEP0040] DeprecationWarning: The `punycode` module is deprecated.
  Please use a userland alternative instead. (Use `node --trace-deprecation ...` to show where the warning was created)

  test: worker test when packaged into executable using pkg
  at:
    line: 983
    column: 15
    file: node:internal/errors
    function: genericNodeError
  code: 2
  killed: false
  signal: null
  cmd: npx pkg /Users/mzasso/git/forks/pino with spaces/test/pkg/index.js --config
    /Users/mzasso/git/forks/pino with spaces/test/pkg/pkg.config.json
  stdout: "> pkg@6.1.1

    > \e[31mError!\e[39m Not more than one entry file/directory is expected\n"
  stderr: >
    (node:8620) [DEP0040] DeprecationWarning: The `punycode` module is deprecated.
    Please use a userland alternative instead.

    (Use `node --trace-deprecation ...` to show where the warning was created)
  tapCaught: returnedPromiseRejection

@targos
Copy link
Contributor Author

targos commented Dec 23, 2024

/cc @mcollina It would be nice if we could have a release after this to have a green module in citgm.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@targos
Copy link
Contributor Author

targos commented Dec 23, 2024

Damn, on Windows I guess it would have to be npx.cmd. I can either make the executable file conditional or pass { shell: true }. What do you prefer?

@mcollina
Copy link
Member

either works, or you can just skip this in citgm.

@mcollina
Copy link
Member

let's just skip this test if CITGM is set

@targos
Copy link
Contributor Author

targos commented Dec 23, 2024

It doesn't only fix citgm. It's quite common on Windows to have folders with spaces

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit abd6621 into pinojs:main Dec 23, 2024
12 checks passed
@targos targos deleted the fix-test-run branch December 23, 2024 10:13
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants