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

test: add escapePOSIXShell util #55125

Merged
merged 12 commits into from
Sep 29, 2024
Merged

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Sep 25, 2024

In #55028, I've tried to come up with an execNode util, but I couldn't find an implementation that would work for all tests, so I ended up having similar-but-different implementations scattered among several tests. After a bit of soul searching, I realized we could take advantage of string templating and have a util that would escape paths or more when we want to pass them to shell.
Of course, that only works for POSIX shells, where the way we use variables in standardized, however since " is not a valid char in Windows-paths, and paths are (almost?) always the only arbitrary value we pass to shells in tests, it should do just fine.

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Sep 25, 2024
Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.24%. Comparing base (0f7bdcc) to head (e0a22b5).
Report is 374 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55125      +/-   ##
==========================================
- Coverage   88.25%   88.24%   -0.02%     
==========================================
  Files         651      651              
  Lines      183856   183863       +7     
  Branches    35854    35827      -27     
==========================================
- Hits       162260   162247      -13     
- Misses      14883    14929      +46     
+ Partials     6713     6687      -26     

see 83 files with indirect coverage changes

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

Copy link
Contributor

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

🙌 yay for code quality improvements!

test/async-hooks/test-callback-error.js Show resolved Hide resolved
test/async-hooks/test-callback-error.js Outdated Show resolved Hide resolved
test/async-hooks/test-callback-error.js Outdated Show resolved Hide resolved
test/parallel/test-fs-readfilesync-pipe-large.js Outdated Show resolved Hide resolved
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 29, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 29, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

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

This looks neat. I have a few (non-blocking) concerns:

  1. Are there plans to change paths on CI machines to something weird, so this issue can be caught immediately? Otherwise we might end up in a situation where new tests will potentially break this again.
  2. This utility should probably be documented in the Common module API part. Otherwise searching for good usage example through /test/parallel/ would be a pain: some are using opts implicitly, some explicitly, existence of opts depends on platform. Also, tagged templates aren't very common, so it would be nice to show an usage example.
  3. Are there any caveats that should be documented? For example, potentially polluting env with the ESCAPED_#s?

test/parallel/test-permission-allow-child-process-cli.js Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor Author

aduh95 commented Sep 29, 2024

  1. Are there plans to change paths on CI machines to something weird, so this issue can be caught immediately?

Yes

3. Are there any caveats that should be documented? For example, potentially polluting env with the ESCAPED_#s?

🤷‍♂️ I guess we'll find out if that's causing problems, it's hard to foresee.

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Sep 29, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 29, 2024
@nodejs-github-bot nodejs-github-bot merged commit 99e0d0d into nodejs:main Sep 29, 2024
57 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 99e0d0d

@aduh95 aduh95 deleted the escapePOSIXShell branch September 29, 2024 20:45
targos pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #55125
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
targos pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #55125
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@aduh95 aduh95 mentioned this pull request Oct 9, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
PR-URL: nodejs#55125
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@marco-ippolito marco-ippolito added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants