Skip to content

Conversation

@JakobJingleheimer
Copy link
Member

@JakobJingleheimerJakobJingleheimer commented Jun 26, 2025

The current message contains a period within the quotes around the specifier, making it look malformed.

@JakobJingleheimerJakobJingleheimer added fast-track PRs that do not need to wait for 48 hours to land. test_runner Issues and PRs related to the test runner subsystem. labels Jun 26, 2025
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@github-actions
Copy link
Contributor

Fast-track has been requested by @JakobJingleheimer. Please 👍 to approve.

@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Jun 26, 2025
@codecov
Copy link

codecovbot commented Jun 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.10%. Comparing base (b4c5fb4) to head (894aad1).
Report is 2 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #58840 +/- ## ========================================== + Coverage 90.08% 90.10% +0.02%  ========================================== Files 640 640 Lines 188446 188446 Branches 36960 36966 +6 ========================================== + Hits 169757 169798 +41 + Misses 11412 11359 -53 - Partials 7277 7289 +12 
Files with missing linesCoverage Δ
lib/internal/test_runner/mock/mock.js99.27% <100.00%> (ø)

... and 38 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JakobJingleheimerJakobJingleheimer added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2025
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2025
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@tniessentniessen left a comment

Choose a reason for hiding this comment

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

FWIW, having punctuation inside quotes is the writing style I have been taught, but I think marking an identifier using '...' does not qualify as a quote, so LGTM.

@JakobJingleheimer
Copy link
MemberAuthor

JakobJingleheimer commented Jun 26, 2025

Sooo the rule for this is when the quoted text is a complete sentence that has its own terminal punctuation:

I told him "I'm hungry!".

When it's not, it doesn't:

It's called "fashion".

In both cases, there is always terminal punctuation after the quotes 🙂

EDIT: But yes, I think the more important issue is not reporting the specifier in a misleading way. Cannot mock 'nonexistent.mjs.' The module is already mocked. makes the specifier look like it contains a trailing . when it actually doesn't.

@JakobJingleheimerJakobJingleheimer added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 26, 2025
@nodejs-github-botnodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 26, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/58840 ✔ Done loading data for nodejs/node/pull/58840 ----------------------------------- PR info ------------------------------------ Title test_runner: correct "already mocked" error punctuation placement (#58840) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch JakobJingleheimer:testrunner/fix/already-mocked-error-message -> nodejs:main Labels fast-track, needs-ci, test_runner Commits 1 - test_runner: correct "already mocked" error punctuation placement Committers 1 - Jacob Smith <[email protected]> PR-URL: https://github.com/nodejs/node/pull/58840 Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/58840 Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> -------------------------------------------------------------------------------- ℹ This PR was created on Thu, 26 Jun 2025 10:01:34 GMT ✔ Approvals: 3 ✔ - Pietro Marchini (@pmarchini): https://github.com/nodejs/node/pull/58840#pullrequestreview-2961537077 ✔ - Ethan Arrowood (@Ethan-Arrowood): https://github.com/nodejs/node/pull/58840#pullrequestreview-2962262634 ✔ - Tobias Nießen (@tniessen) (TSC): https://github.com/nodejs/node/pull/58840#pullrequestreview-2962468027 ℹ This PR is being fast-tracked ✘ This PR needs to wait 38 more hours to land (or 0 hours if there is 1 more approval (👍) of the fast-track request from collaborators). ✔ Last GitHub CI successful ℹ Last Full PR CI on 2025-06-26T14:13:20Z: https://ci.nodejs.org/job/node-test-pull-request/67666/ - Querying data for job/node-test-pull-request/67666/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/15910286689

@JakobJingleheimer
Copy link
MemberAuthor

Ugh. Could I get 1 more 👍 on the fast-track please

@lpincalpinca added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jun 26, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 26, 2025
@nodejs-github-botnodejs-github-bot merged commit 5b0c7db into nodejs:mainJun 26, 2025
81 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 5b0c7db

targos pushed a commit that referenced this pull request Jul 3, 2025
PR-URL: #58840 Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
richardlau pushed a commit that referenced this pull request Sep 20, 2025
PR-URL: #58840 Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Nov 19, 2025
PR-URL: #58840 Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Nov 19, 2025
PR-URL: #58840 Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fast-trackPRs that do not need to wait for 48 hours to land.needs-ciPRs that need a full CI run.test_runnerIssues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@JakobJingleheimer@nodejs-github-bot@lpinca@tniessen@Ethan-Arrowood@pmarchini@gurgunday