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.

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

test: refactor test-esm-loader-hooks for easier debugging #49131

Merged
merged 1 commit into from
Aug 15, 2023
Merged

test: refactor test-esm-loader-hooks for easier debugging #49131

merged 1 commit into from
Aug 15, 2023

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Aug 13, 2023 •
edited
Loading

  • Always check stderr before stdout as the former would contain error information.
  • Always match the full stdout to avoid surprises.
  • Use deepStrictEqual when appropriate to get more informative test failures.
  • Remove leading slashes from relative paths/URLs to not confuse them with absolute paths.
  • Remove unnecessary --no-warnings flag.

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Aug 13, 2023
@aduh95
test: refactor test-esm-loader-hooks for easier debugging
3a42c8e 
- Always check stderr before stdout as the former would contain error
  information.
- Always match the full stdout to avoid surprises.
- Use `deepStrictEqual` when appropriate to get more informative test
  failures.
- Remove leading slashes from relative paths/URLs to not confuse them
  with absolute paths.
- Remove unnecessary `--no-warnings` flag.
@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 Aug 13, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 13, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53270/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53278/

GeoffreyBooth
GeoffreyBooth approved these changes Aug 13, 2023
View reviewed changes
Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

Maybe also split this into multiple files? (Or in a separate PR.)

@aduh95
Copy link
Contributor Author

aduh95 commented Aug 13, 2023

I think having one file is better, it helps with consistency (I would almost certainly not have made this PR if those tests were each in their own file, because I would simply not have seen the issue).

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53285/

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 13, 2023
This was referenced Aug 14, 2023
Open
Open
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 15, 2023
@nodejs-github-bot nodejs-github-bot merged commit 1aeb48c into nodejs:main Aug 15, 2023
30 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 1aeb48c

@aduh95 aduh95 deleted the test-esm-hooks branch August 15, 2023 12:16
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
@aduh95 @RafaelGSS
test: refactor test-esm-loader-hooks for easier debugging
b94d0f9 
- Always check stderr before stdout as the former would contain error
  information.
- Always match the full stdout to avoid surprises.
- Use `deepStrictEqual` when appropriate to get more informative test
  failures.
- Remove leading slashes from relative paths/URLs to not confuse them
  with absolute paths.
- Remove unnecessary `--no-warnings` flag.

PR-URL: #49131
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
Merged
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Aug 15, 2023
@aduh95 @RafaelGSS
test: refactor test-esm-loader-hooks for easier debugging
0de3403 
- Always check stderr before stdout as the former would contain error
  information.
- Always match the full stdout to avoid surprises.
- Use `deepStrictEqual` when appropriate to get more informative test
  failures.
- Remove leading slashes from relative paths/URLs to not confuse them
  with absolute paths.
- Remove unnecessary `--no-warnings` flag.

PR-URL: nodejs#49131
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
rluvaton pushed a commit to rluvaton/node that referenced this pull request Aug 15, 2023
@aduh95 @rluvaton
test: refactor test-esm-loader-hooks for easier debugging
e5dbab9 
- Always check stderr before stdout as the former would contain error
  information.
- Always match the full stdout to avoid surprises.
- Use `deepStrictEqual` when appropriate to get more informative test
  failures.
- Remove leading slashes from relative paths/URLs to not confuse them
  with absolute paths.
- Remove unnecessary `--no-warnings` flag.

PR-URL: nodejs#49131
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@github-actions github-actions bot mentioned this pull request Aug 16, 2023
Open
24 tasks
RafaelGSS pushed a commit that referenced this pull request Aug 16, 2023
@aduh95 @RafaelGSS
test: refactor test-esm-loader-hooks for easier debugging
3939a01 
- Always check stderr before stdout as the former would contain error
  information.
- Always match the full stdout to avoid surprises.
- Use `deepStrictEqual` when appropriate to get more informative test
  failures.
- Remove leading slashes from relative paths/URLs to not confuse them
  with absolute paths.
- Remove unnecessary `--no-warnings` flag.

PR-URL: #49131
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
@aduh95 @RafaelGSS
test: refactor test-esm-loader-hooks for easier debugging
1d17c10 
- Always check stderr before stdout as the former would contain error
  information.
- Always match the full stdout to avoid surprises.
- Use `deepStrictEqual` when appropriate to get more informative test
  failures.
- Remove leading slashes from relative paths/URLs to not confuse them
  with absolute paths.
- Remove unnecessary `--no-warnings` flag.

PR-URL: #49131
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023
@aduh95 @targos
test: refactor test-esm-loader-hooks for easier debugging
1c020cc 
- Always check stderr before stdout as the former would contain error
  information.
- Always match the full stdout to avoid surprises.
- Use `deepStrictEqual` when appropriate to get more informative test
  failures.
- Remove leading slashes from relative paths/URLs to not confuse them
  with absolute paths.
- Remove unnecessary `--no-warnings` flag.

PR-URL: nodejs#49131
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@targos targos mentioned this pull request Nov 11, 2023
Closed
@targos targos added the backported-to-v18.x PRs backported to the v18.x-staging branch. label Nov 23, 2023
targos pushed a commit that referenced this pull request Nov 23, 2023
@aduh95 @targos
test: refactor test-esm-loader-hooks for easier debugging
e8d475f 
- Always check stderr before stdout as the former would contain error
  information.
- Always match the full stdout to avoid surprises.
- Use `deepStrictEqual` when appropriate to get more informative test
  failures.
- Remove leading slashes from relative paths/URLs to not confuse them
  with absolute paths.
- Remove unnecessary `--no-warnings` flag.

PR-URL: #49131
Backport-PR-URL: #50669
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@targos targos mentioned this pull request Nov 28, 2023
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test: refactor test-esm-loader-hooks for easier debugging
cbbefca 
- Always check stderr before stdout as the former would contain error
  information.
- Always match the full stdout to avoid surprises.
- Use `deepStrictEqual` when appropriate to get more informative test
  failures.
- Remove leading slashes from relative paths/URLs to not confuse them
  with absolute paths.
- Remove unnecessary `--no-warnings` flag.

PR-URL: nodejs/node#49131
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test: refactor test-esm-loader-hooks for easier debugging
f3b2f78 
- Always check stderr before stdout as the former would contain error
  information.
- Always match the full stdout to avoid surprises.
- Use `deepStrictEqual` when appropriate to get more informative test
  failures.
- Remove leading slashes from relative paths/URLs to not confuse them
  with absolute paths.
- Remove unnecessary `--no-warnings` flag.

PR-URL: nodejs/node#49131
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atlowChemi atlowChemi atlowChemi approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@JakobJingleheimer JakobJingleheimer Awaiting requested review from JakobJingleheimer

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backported-to-v18.x PRs backported to the v18.x-staging branch. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aduh95 @nodejs-github-bot @GeoffreyBooth @atlowChemi @targos