fix: incorrect timers-promisified test case

[ttzztztz]

This test case is incorrect, and it caused too many unit-test failures

https://ci.nodejs.org/job/node-test-commit-linuxone/nodes=rhel7-s390x/25697/testReport/
https://ci.nodejs.org/job/node-test-commit-linuxone/nodes=rhel7-s390x/25708/testReport/
...

You can see it in this page: https://ci.nodejs.org/job/node-test-commit-linuxone/nodes=rhel7-s390x/
This is probably caused by a wrong time-sequence execution order. I'm trying to work on this.

Analysis

  setPromiseTimeout(1).then(() => pre = true);  // L1
  const iterable = setInterval(() => {}, 2); // L2
  //...
  iterator.next().then(...); // L3
  setPromiseTimeout(3).then(() => post = true); // L4

Assume we execute this code at time 0
Time=0, FinishExecute=L1, callback L1 is queued to execute at Time=1
Time=1, FinishExecute=L2, callback L2 is queued to execute at Time=3, 5, 7, ...
Time=2, FinishExecute=L3
Time=11, FinishExecute=L4, callback L3 is queued to execute at Time=14

In this case, this unit test will throw an error 'second interval ran too early', but it is OK.

My solution

First, the timeout is 10, 20, 30 to tolerate the time for code-execution
Second, use the Promise.all to wait for these three promises

I think it would work.

Fixes: #37395

[Trott]

@benjamingr @Linkgoron @nodejs/timers

[Trott]

Refs: #37395

[Linkgoron]

In retrospect, I'm not 100% sure that the test is really necessary, as there are already some timing tests.

However, while the test fix looks fine, I'm not sure what the policy is about test performance though, is ~400ms for a test OK?

[ttzztztz]

In retrospect, I'm not 100% sure that the test is really necessary, as there are already some timing tests.

However, while the test fix looks fine, I'm not sure what the policy is about test performance though, is ~400ms for a test OK?

After looking at other test cases, I found this

const TIMEOUT = common.platformTimeout(100)

The definition of the function platformTimeout is

function platformTimeout(ms) {
  const multipliers = typeof ms === 'bigint' ?
    { two: 2n, four: 4n, seven: 7n } : { two: 2, four: 4, seven: 7 };

  if (process.features.debug)
    ms = multipliers.two * ms;

  if (isAIX)
    return multipliers.two * ms; // Default localhost speed is slower on AIX

  if (process.arch !== 'arm')
    return ms;

  const armv = process.config.variables.arm_version;

  if (armv === '6')
    return multipliers.seven * ms;  // ARMv6

  if (armv === '7')
    return multipliers.two * ms;  // ARMv7

  return ms; // ARMv8+
}

400ms is a long time, I admit. So maybe I can adjust it to common.platformTimeout(...) so that the timeout can be adjusted in different platforms.

[aduh95]

nit

[aduh95]

LinuxOne CI is passing: https://ci.nodejs.org/job/node-test-commit-linuxone/nodes=rhel7-s390x/25734/

400ms is a long time, I admit. So maybe I can adjust it to common.platformTimeout(...) so that the timeout can be adjusted in different platforms.

That's sounds like a solid plan! Can you make this change?

[ttzztztz]

LinuxOne CI is passing: https://ci.nodejs.org/job/node-test-commit-linuxone/nodes=rhel7-s390x/25734/

400ms is a long time, I admit. So maybe I can adjust it to common.platformTimeout(...) so that the timeout can be adjusted in different platforms.

That's sounds like a solid plan! Can you make this change?

Implemented 51921f73890790322f58c1b263265c39d10c9c01, waiting for the test result.

[ttzztztz]

@aduh95 can you run the test-linuxone test for the new changes once again?

[nodejs-github-bot]

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

[aduh95]

Fast-track?

[Trott]

Optional nit: Since the problem we've been seeing is with the test being flaky on a fast machine, I wonder if common.platformTimeout() isn't needed here. It's usually for giving slow machines more time, but the problem we're seeing right now is the other way around--our LinuxONE host is too efficient.

[Trott]

@nodejs/timers

[nodejs-github-bot]

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

[targos]

Landed in 63794b4