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

lib: fix module print timing when specifier includes " #55150

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Sep 28, 2024

Currently, we generate invalid JSON if the specifier contains ", this can be addressed by escaping all double quotes like this PR is doing.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Sep 28, 2024
Copy link

codecov bot commented Sep 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.18%. Comparing base (f5d454a) to head (bda9e62).
Report is 351 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55150      +/-   ##
==========================================
- Coverage   88.23%   88.18%   -0.05%     
==========================================
  Files         651      649       -2     
  Lines      183902   183327     -575     
  Branches    35855    35686     -169     
==========================================
- Hits       162269   161672     -597     
- Misses      14926    15024      +98     
+ Partials     6707     6631      -76     
Files with missing lines Coverage Δ
lib/internal/util/debuglog.js 95.20% <100.00%> (ø)

... and 56 files with indirect coverage changes

@H4ad
Copy link
Member

H4ad commented Sep 28, 2024

From what I remember, almost all places that use/expose trace labels are susceptible to this issue, I don't remember exactly the places but I think could be something worthy to take a double look.

Fixing this implementation also fixes the console.time implementation, so if you want to include a test there too, could be useful.

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 28, 2024

Fixing this implementation also fixes the console.time implementation, so if you want to include a test there too, could be useful.

Do you have an example? Naive testing (node -e 'console.time(`test"`);console.timeEnd(`test"`)') doesn't show the issue.

@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

@nodejs-github-bot
Copy link
Collaborator

@H4ad
Copy link
Member

H4ad commented Sep 30, 2024

@aduh95 node --trace-event-categories -e 'console.time(`test"`);console.timeEnd(`test"`)'

The generated logs are in json, and the json file is broken.

Edit2: I'm okay with adding tests in another PR (since the CI of this PR is already green)

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 8, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 8, 2024
@nodejs-github-bot nodejs-github-bot merged commit 78f421d into nodejs:main Oct 8, 2024
61 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 78f421d

@aduh95 aduh95 deleted the quote-print-timing branch October 8, 2024 17:19
aduh95 added a commit that referenced this pull request Oct 9, 2024
PR-URL: #55150
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
@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#55150
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
@marco-ippolito marco-ippolito added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label Nov 16, 2024
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
PR-URL: nodejs#55150
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants