test_runner: fix `duration_ms` to be milliseconds by MoLow · Pull Request #44450 · nodejs/node · GitHub

result of running node -e "require('node:test').test((t, done)=>setTimeout(() => done(), 500))"
before:

TAP version 13
# Subtest: <anonymous>
ok 1 - <anonymous>
  ---
  duration_ms: 0.501664977
  ...
1..1
# tests 1
# pass 1
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 0.562913893

after:

TAP version 13
# Subtest: <anonymous>
ok 1 - <anonymous>
  ---
  duration_ms: 501.590752
  ...
1..1
# tests 1
# pass 1
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 530.935436

not sure whether this was intended or not

CC @nodejs/test_runner

I am also not really sure what would be a reliable (and not flaky) way to test this

platform timeouts are an OK way to deal with the flakiness here but not a perfect one. We don't have fake timer infrastructure as far as I know.

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

I'm not opposed to changing this, but this was done intentionally to match what appears to be the behavior of some existing systems - jenkinsci/tap-plugin#6 (comment).

Dismissing due to @cjihrig comment. I don't have a strong opinion.

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

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

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

I'm pretty sure this contradicts the TAP specification and will break interoperability with, for example, Jenkins plugins and whatnot. Checking....

Looks like duration is not part of the specification so this would not violate it, but I would encourage caution nonetheless. But if this is an experimental feature, I won't block a breaking change. Because: experimental.

I'm -0.5. Won't block, but I will be surprised if this does not introduce interoperability problems with Jenkins plugins and other things. Would love to be wrong about that because duration_ms is a terrible label for something that is actually in seconds and everyone always trips up when reading it.

@Trott It seems the official Jenkins TAP plugin expects milliseconds, am I wrong? https://plugins.jenkins.io/tap/#plugin-content-125-2016-06-21

FWIW, this PR is not very important to me - I only fixed something that I found very confusing. we can add a comment instead.
I am ambivalent regarding the correct thing to do as well, but I think if we decide to favor fixing this historical mistake (that doesn't seem to be specified anywhere) - the correct time to do it is while test runner is still experimental.

@Trott It seems the official Jenkins TAP plugin expects milliseconds, am I wrong? https://plugins.jenkins.io/tap/#plugin-content-125-2016-06-21

It sure looks like it. 👍

PR-URL: #44450 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

hey, sorry i haven't read all the context on this, so this is just a drive-by on noticing it, but you might want to search a bit more for history on this -- duration_ms has always had to be interpreted as "duration in seconds with milliseconds" - we've changed this in multiple places, multiple times, because of the bad naming, but always ended up breaking stuff.. so maybe you can push through a comprehensive change but just FYI this has hurt us badly in the past trying to make the name match what we think it should be

(I think this applies fairly broadly to other TAP implementations, not just restricted to our stack, so my point being - tread carefully)

@rvagg thanks for the feedback,
It seems like some other plugins have already fixed this but it is indeed hard to determine what this can affect.
Do you know what kind of issues this caused in the previous attempts to fix it?

PR-URL: #44450 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

This depends on #44241

PR-URL: nodejs#44450 Backport-PR-URL: nodejs#44873 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

PR-URL: #44450 Backport-PR-URL: #44873 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [node](https://github.com/nodejs/node) | stage | minor | `16.18.1-bullseye` -> `16.19.0-bullseye` | --- ### Release Notes <details> <summary>nodejs/node</summary> ### [`v16.19.0`](https://github.com/nodejs/node/releases/tag/v16.19.0): 2022-12-13, Version 16.19.0 'Gallium' (LTS), @richardlau [Compare Source](nodejs/node@v16.18.1...v16.19.0) ##### Notable Changes ##### OpenSSL 1.1.1s This update is a bugfix release and does not address any security vulnerabilities. ##### Root certificates updated to NSS 3.85 Certificates added: - Autoridad de Certificacion Firmaprofesional CIF [`A626340`](nodejs/node@A62634068) - Certainly Root E1 - Certainly Root R1 - D-TRUST BR Root CA 1 2020 - D-TRUST EV Root CA 1 2020 - DigiCert TLS ECC P384 Root G5 - DigiCert TLS RSA4096 Root G5 - E-Tugra Global Root CA ECC v3 - E-Tugra Global Root CA RSA v3 - HiPKI Root CA - G1 - ISRG Root X2 - Security Communication ECC RootCA1 - Security Communication RootCA3 - Telia Root CA v2 - vTrus ECC Root CA - vTrus Root CA Certificates removed: - Cybertrust Global Root - DST Root CA X3 - GlobalSign Root CA - R2 - Hellenic Academic and Research Institutions RootCA 2011 ##### Time zone update to 2022f Time zone data has been updated to 2022f. This includes changes to Daylight Savings Time (DST) for Fiji and Mexico. For more information, see <https://mm.icann.org/pipermail/tz-announce/2022-October/000075.html>. ##### Other Notable Changes - \[[`33707dcd03`](nodejs/node@33707dcd03)] - **dgram**: add dgram send queue info (theanarkh) [#44149](nodejs/node#44149) Dependency updates: - \[[`3b2b70d792`](nodejs/node@3b2b70d792)] - **deps**: upgrade npm to 8.19.3 (npm team) [#45322](nodejs/node#45322) Experimental features: - \[[`1e0dcd1ee0`](nodejs/node@1e0dcd1ee0)] - **cli**: add `--watch` (Moshe Atlow) [#44366](nodejs/node#44366) - \[[`8c73279ebb`](nodejs/node@8c73279ebb)] - **util**: add default value option to parsearg (Manuel Spigolon) [#44631](nodejs/node#44631) ##### Commits - \[[`bbef3c42f6`](nodejs/node@bbef3c42f6)] - **build**: add version info to timezone update PR (Darshan Sen) [#45021](nodejs/node#45021) - \[[`cc2c7648e0`](nodejs/node@cc2c7648e0)] - **build**: support Python 3.11 (Luigi Pinca) [#45191](nodejs/node#45191) - \[[`ac24c80663`](nodejs/node@ac24c80663)] - **build**: remove redundant condition from common.gypi (Richard Lau) [#45076](nodejs/node#45076) - \[[`03dcbe3030`](nodejs/node@03dcbe3030)] - **build**: fix bad upstream merge (Stephen Gallagher) [#44642](nodejs/node#44642) - \[[`1e0dcd1ee0`](nodejs/node@1e0dcd1ee0)] - **cli**: add `--watch` (Moshe Atlow) [#44366](nodejs/node#44366) - \[[`96d131665e`](nodejs/node@96d131665e)] - **cluster**: use inspector utils (Moshe Atlow) [#44592](nodejs/node#44592) - \[[`704836033a`](nodejs/node@704836033a)] - **crypto**: update root certificates (Luigi Pinca) [#45490](nodejs/node#45490) - \[[`5a776d4a69`](nodejs/node@5a776d4a69)] - **deps**: update timezone to 2022f (Richard Lau) [#45613](nodejs/node#45613) - \[[`3b2b70d792`](nodejs/node@3b2b70d792)] - **deps**: upgrade npm to 8.19.3 (npm team) [#45322](nodejs/node#45322) - \[[`9fbc8b21db`](nodejs/node@9fbc8b21db)] - **deps**: update corepack to 0.15.1 (Node.js GitHub Bot) [#45331](nodejs/node#45331) - \[[`87e3d002ca`](nodejs/node@87e3d002ca)] - **deps**: update corepack to 0.15.0 (Node.js GitHub Bot) [#45235](nodejs/node#45235) - \[[`e972ff7b13`](nodejs/node@e972ff7b13)] - **deps**: V8: backport [`bbd800c`](nodejs/node@bbd800c6e359) (Chengzhong Wu) [#44947](nodejs/node#44947) - \[[`af9d8217c0`](nodejs/node@af9d8217c0)] - **deps**: V8: cherry-pick [`b953542`](nodejs/node@b95354290941) (Chengzhong Wu) [#44947](nodejs/node#44947) - \[[`38202d321b`](nodejs/node@38202d321b)] - **deps**: update undici to 5.12.0 (Node.js GitHub Bot) [#45236](nodejs/node#45236) - \[[`7c0da6adf9`](nodejs/node@7c0da6adf9)] - **deps**: update archs files for OpenSSL-1.1.1s (RafaelGSS) [#45274](nodejs/node#45274) - \[[`1149ead6f7`](nodejs/node@1149ead6f7)] - **deps**: upgrade openssl sources to OpenSSL\_1\_1\_1s (RafaelGSS) [#45274](nodejs/node#45274) - \[[`cd54bce4f5`](nodejs/node@cd54bce4f5)] - **deps**: update timezone (Node.js GitHub Bot) [#44950](nodejs/node#44950) - \[[`2901abe4f0`](nodejs/node@2901abe4f0)] - **deps**: update undici to 5.11.0 (Node.js GitHub Bot) [#44929](nodejs/node#44929) - \[[`c80cf97033`](nodejs/node@c80cf97033)] - **deps**: update corepack to 0.14.2 (Node.js GitHub Bot) [#44775](nodejs/node#44775) - \[[`33707dcd03`](nodejs/node@33707dcd03)] - **dgram**: add dgram send queue info (theanarkh) [#44149](nodejs/node#44149) - \[[`c708d9bb94`](nodejs/node@c708d9bb94)] - **doc**: fix typo in parseArgs default value (Tobias Nießen) [#45083](nodejs/node#45083) - \[[`5a0efa05d2`](nodejs/node@5a0efa05d2)] - **node-api**: handle no support for external buffers (Michael Dawson) [#45181](nodejs/node#45181) - \[[`db31de634e`](nodejs/node@db31de634e)] - **readline**: refactor to avoid unsafe regex primordials (Antoine du Hamel) [#43475](nodejs/node#43475) - \[[`fbc52e5729`](nodejs/node@fbc52e5729)] - **src**: disambiguate terms used to refer to builtins and addons (Joyee Cheung) [#44135](nodejs/node#44135) - \[[`953072d3db`](nodejs/node@953072d3db)] - **src**: let http2 streams end after session close (Santiago Gimeno) [#45153](nodejs/node#45153) - \[[`54608d8dc3`](nodejs/node@54608d8dc3)] - **src**: split property helpers from node::Environment (Chengzhong Wu) [#44056](nodejs/node#44056) - \[[`6733556783`](nodejs/node@6733556783)] - **test**: add test to validate changelogs for releases (Richard Lau) [#45325](nodejs/node#45325) - \[[`821d832cef`](nodejs/node@821d832cef)] - **test**: mark test-watch-mode\* as flaky on all platforms (Pierrick Bouvier) [#45049](nodejs/node#45049) - \[[`02a18eac69`](nodejs/node@02a18eac69)] - **test**: fix test-runner-inspect (Moshe Atlow) [#44620](nodejs/node#44620) - \[[`197df63f74`](nodejs/node@197df63f74)] - **test**: add a test to ensure the correctness of timezone upgrades (Darshan Sen) [#45299](nodejs/node#45299) - \[[`42e9d8016a`](nodejs/node@42e9d8016a)] - **test**: fix textdecoder test for small-icu builds (Richard Lau) [#45225](nodejs/node#45225) - \[[`6d736a56d8`](nodejs/node@6d736a56d8)] - **test**: fix watch mode test flake (Moshe Atlow) [#44739](nodejs/node#44739) - \[[`543d3d2bf3`](nodejs/node@543d3d2bf3)] - **test**: deflake watch mode tests (Moshe Atlow) [#44621](nodejs/node#44621) - \[[`97f6caf4eb`](nodejs/node@97f6caf4eb)] - **test**: split watch mode inspector tests to sequential (Moshe Atlow) [#44551](nodejs/node#44551) - \[[`499750ff7a`](nodejs/node@499750ff7a)] - **test**: update list of known globals (Antoine du Hamel) [#45255](nodejs/node#45255) - \[[`64d343af74`](nodejs/node@64d343af74)] - **test_runner**: support using `--inspect` with `--test` (Moshe Atlow) [#44520](nodejs/node#44520) - \[[`99ee5e484d`](nodejs/node@99ee5e484d)] - **test_runner**: fix `duration_ms` to be milliseconds (Moshe Atlow) [#44450](nodejs/node#44450) - \[[`37e909251c`](nodejs/node@37e909251c)] - **test_runner**: support programmatically running `--test` (Moshe Atlow) [#44241](nodejs/node#44241) - \[[`0ae5694f88`](nodejs/node@0ae5694f88)] - **tools**: update certdata.txt (Luigi Pinca) [#45490](nodejs/node#45490) - \[[`891368cefd`](nodejs/node@891368cefd)] - **tools**: remove faulty early termination logic from update-timezone.mjs (Darshan Sen) [#44870](nodejs/node#44870) - \[[`543493c242`](nodejs/node@543493c242)] - **tools**: fix timezone update tool (Darshan Sen) [#44870](nodejs/node#44870) - \[[`c77f660b75`](nodejs/node@c77f660b75)] - **tools**: fix `create-or-update-pull-request-action` hash on GHA (Antoine du Hamel) [#45166](nodejs/node#45166) - \[[`58c30dd049`](nodejs/node@58c30dd049)] - **tools**: update gr2m/create-or-update-pull-request-action (Luigi Pinca) [#45022](nodejs/node#45022) - \[[`749a4b3e5e`](nodejs/node@749a4b3e5e)] - **tools**: use Python 3.11 in GitHub Actions workflows (Luigi Pinca) [#45191](nodejs/node#45191) - \[[`6f541d99a5`](nodejs/node@6f541d99a5)] - **tools**: have test-asan use ubuntu-20.04 (Filip Skokan) [#45581](nodejs/node#45581) - \[[`e7ed56f501`](nodejs/node@e7ed56f501)] - **tools**: make license-builder.sh comply with shellcheck 0.8.0 (Rich Trott) [#41258](nodejs/node#41258) - \[[`cc819b4bf8`](nodejs/node@cc819b4bf8)] - **tools**: fix typo in `avoid-prototype-pollution` lint rule (Antoine du Hamel) [#44446](nodejs/node#44446) - \[[`254358c81e`](nodejs/node@254358c81e)] - **tools**: refactor `avoid-prototype-pollution` lint rule (Antoine du Hamel) [#43476](nodejs/node#43476) - \[[`8c73279ebb`](nodejs/node@8c73279ebb)] - **util**: add default value option to parsearg (Manuel Spigolon) [#44631](nodejs/node#44631) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.walbeck.it/mwalbeck/docker-cyberchef/pulls/143 Co-authored-by: renovate-bot <bot@walbeck.it> Co-committed-by: renovate-bot <bot@walbeck.it>

PR-URL: nodejs/node#44450 Backport-PR-URL: nodejs/node#44873 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

PR-URL: nodejs/node#44450 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> (cherry picked from commit 6ee1f3444f8c1cf005153f936ffc74221d55658b)

test_runner: fix duration_ms to be milliseconds

b04ead1

MoLow requested a review from cjihrig August 30, 2022 08:15

nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Aug 30, 2022

benjamingr approved these changes Aug 30, 2022

View reviewed changes

aduh95 approved these changes Aug 30, 2022

View reviewed changes

MoLow added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Aug 30, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2022

lpinca previously approved these changes Aug 30, 2022

View reviewed changes

github-actions Bot mentioned this pull request Aug 31, 2022

CI Reliability 2022-08-31 nodejs/reliability#363

Open

30 tasks

Trott previously requested changes Aug 31, 2022

View reviewed changes

fhinkel approved these changes Aug 31, 2022

View reviewed changes

github-actions Bot mentioned this pull request Sep 1, 2022

CI Reliability 2022-09-01 nodejs/reliability#364

Open

28 tasks

Trott approved these changes Sep 1, 2022

View reviewed changes

github-actions Bot mentioned this pull request Sep 2, 2022

CI Reliability 2022-09-02 nodejs/reliability#365

Open

24 tasks

github-actions Bot mentioned this pull request Sep 6, 2022

CI Reliability 2022-09-06 nodejs/reliability#369

Open

17 tasks

juanarbol added the backport-blocked-v16.x label Oct 3, 2022

MoLow mentioned this pull request Oct 3, 2022

[v16.x backport] test runner additions #44873

Closed

MoLow added backport-open-v16.x and removed backport-blocked-v16.x labels Oct 3, 2022

richardlau added backported-to-v16.x and removed backport-open-v16.x labels Dec 7, 2022

richardlau mentioned this pull request Dec 8, 2022

v16.19.0 proposal #45791

Merged

opalmedapps-bot mentioned this pull request Jan 28, 2025

chore: update node.js to v16.19.0 opalmedapps/docs#55

Closed

1 task

test_runner: fix `duration_ms` to be milliseconds by MoLow · Pull Request #44450 · nodejs/node · GitHub

Uh oh!

Uh oh!

test_runner: fix duration_ms to be milliseconds - #44450

test_runner: fix duration_ms to be milliseconds#44450nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom MoLow:test-runner-fix-duration_msMoLow/node:test-runner-fix-duration_msCopy head branch name to clipboard

Conversation

MoLow commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoLow commented Aug 30, 2022

Uh oh!

MoLow commented Aug 30, 2022

Uh oh!

benjamingr commented Aug 30, 2022

Uh oh!

nodejs-github-bot commented Aug 30, 2022

Uh oh!

cjihrig commented Aug 30, 2022

Uh oh!

nodejs-github-bot commented Aug 30, 2022

Uh oh!

nodejs-github-bot commented Aug 31, 2022

Uh oh!

nodejs-github-bot commented Aug 31, 2022

Uh oh!

Trott left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoLow commented Sep 1, 2022

Uh oh!

MoLow commented Sep 1, 2022

Uh oh!

Trott commented Sep 1, 2022

Uh oh!

rvagg commented Sep 6, 2022

Uh oh!

rvagg commented Sep 6, 2022

Uh oh!

MoLow commented Sep 7, 2022

Uh oh!

juanarbol commented Oct 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

test_runner: fix duration_ms to be milliseconds#44450
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
MoLow:test-runner-fix-duration_ms

MoLow commented Aug 30, 2022 •
edited
Loading

Trott left a comment •
edited
Loading

Trott commented Aug 31, 2022 •
edited
Loading