test: skip test if host is too slow #15688

Trott · 2017-09-29T18:11:22Z

test-http-server-consumed-timeout will fail if the host is sufficiently
loaded that a 25ms interval takes more than 200ms to be invoked. Skip
the test in that situation.

Fixes: #14312

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

test-http-server-consumed-timeout will fail if the host is sufficiently loaded that a 25ms interval takes more than 200ms to be invoked. Skip the test in that situation. Fixes: nodejs#14312

Trott · 2017-09-29T18:12:31Z

The robustness here can be checked by copying the test to test/parallel and invoking it with:

tools/test.py -j 96 --repeat 192 test/parallel/test-http-server-consumed-timeout.js

On master, I get many many failures that way. With this change, no failures.

Trott · 2017-09-29T18:15:24Z

CI stress test against master showing infrequent but still non-zero failures even though the test is in sequential: https://ci.nodejs.org/job/node-stress-single-test/1427/nodes=rhel72-s390x/console

CI stress test against this pull request, hopefully will come back green: https://ci.nodejs.org/job/node-stress-single-test/1428/nodes=rhel72-s390x/console

Trott · 2017-09-29T18:15:42Z

@BridgeAR

refack

Just nits

refack · 2017-09-29T18:52:46Z

test/sequential/test-http-server-consumed-timeout.js

-                 common.mustNotCall('Request timeout should not fire'));
+  req.setTimeout(TIMEOUT, () => {
+    if (!intervalWasInvoked)
+      common.skip('interval was not invoked quickly enough for test');


nit: return common.skip() as to not confuse static analysers

refack · 2017-09-29T18:52:55Z

test/sequential/test-http-server-consumed-timeout.js

+      const now = Date.now();
+      console.log('diff is ' + (now - time));
+      if (time < now - TIMEOUT)
+        common.skip('interval is not invoked quickly enough for test');


nit: return common.skip() as to not confuse static analysers

refack · 2017-09-29T18:54:15Z

test/sequential/test-http-server-consumed-timeout.js

+      // to be invoked, skip the test.
+      const now = Date.now();
+      console.log('diff is ' + (now - time));
+      if (time < now - TIMEOUT)


nit: I find now - time > TIMEOUT more intuitive

Trott · 2017-09-29T22:45:43Z

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

Fishrock123 · 2017-10-02T17:15:01Z

I'm curious - is this a first? A test that skips if it times out?

refack · 2017-10-02T17:22:49Z

@Fishrock123 There is at least one similar case

node/test/async-hooks/test-callback-error.js

Lines 94 to 120 in 95c8503

    
           const tO = setTimeout(() => { 
        
             console.log(stderr); 
        
             child.kill('SIGKILL'); 
        
             process.exit(1); 
        
           }, 15 * 1000); 
        
           tO.unref(); 
        
           child.on('close', (code, signal) => { 
        
             clearTimeout(tO); 
        
             if (common.isWindows) { 
        
               assert.strictEqual(code, 134); 
        
               assert.strictEqual(signal, null); 
        
             } else { 
        
               assert.strictEqual(code, null); 
        
               // most posix systems will show 'SIGABRT', but alpine34 does not 
        
               if (signal !== 'SIGABRT') { 
        
                 console.log(`parent recived signal ${signal}\nchild's stderr:`); 
        
                 console.log(stderr); 
        
                 process.exit(1); 
        
               } 
        
               assert.strictEqual(signal, 'SIGABRT'); 
        
             } 
        
             assert.strictEqual(stdout, ''); 
        
             const firstLineStderr = stderr.split(/[\r\n]+/g)[0].trim(); 
        
             assert.strictEqual(firstLineStderr, 'Error: test_callback_abort'); 
        
           }); 
        
           console.timeEnd('end case 3');

If macOS is not configured properly creating a core dump could timeout so we preemptively timeout

test-http-server-consumed-timeout will fail if the host is sufficiently loaded that a 25ms interval takes more than 200ms to be invoked. Skip the test in that situation. PR-URL: nodejs#15688 Fixes: nodejs#14312 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Trott · 2017-10-04T16:35:24Z

Landed in a3cd8ed

test-http-server-consumed-timeout will fail if the host is sufficiently loaded that a 25ms interval takes more than 200ms to be invoked. Skip the test in that situation. PR-URL: nodejs/node#15688 Fixes: nodejs/node#14312 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

test-http-server-consumed-timeout will fail if the host is sufficiently loaded that a 25ms interval takes more than 200ms to be invoked. Skip the test in that situation. PR-URL: #15688 Fixes: #14312 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

test: skip test if host is too slow

f001398

test-http-server-consumed-timeout will fail if the host is sufficiently loaded that a 25ms interval takes more than 200ms to be invoked. Skip the test in that situation. Fixes: nodejs#14312

nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 29, 2017

Trott mentioned this pull request Sep 29, 2017

investigate flaky "sequential/test-http-server-consumed-timeout" #14312

Closed

refack approved these changes Sep 29, 2017

View reviewed changes

mscdex added the http Issues or PRs related to the http subsystem. label Sep 29, 2017

Trott added 2 commits September 29, 2017 13:12

squash: remove console.log()

28ad47b

squash: refack nits

77ea4bf

Trott force-pushed the fix-14312 branch from cc8b579 to 77ea4bf Compare September 29, 2017 22:41

BridgeAR approved these changes Sep 30, 2017

View reviewed changes

refack approved these changes Sep 30, 2017

View reviewed changes

jasnell approved these changes Oct 1, 2017

View reviewed changes

gibfahn approved these changes Oct 1, 2017

View reviewed changes

Trott closed this Oct 4, 2017

MylesBorins mentioned this pull request Oct 7, 2017

v8.7.0 proposal #15762

Merged

MylesBorins added the land-on-v6.x label Oct 17, 2017

MylesBorins mentioned this pull request Oct 17, 2017

v6.12.0 proposal #16263

Merged

MylesBorins mentioned this pull request Nov 3, 2017

v4.8.6 proposal #16500

Merged

Trott deleted the fix-14312 branch January 13, 2022 22:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: skip test if host is too slow #15688

test: skip test if host is too slow #15688

Trott commented Sep 29, 2017

Trott commented Sep 29, 2017

Trott commented Sep 29, 2017

Trott commented Sep 29, 2017

refack left a comment

refack Sep 29, 2017

refack Sep 29, 2017

refack Sep 29, 2017

Trott commented Sep 29, 2017

Fishrock123 commented Oct 2, 2017

refack commented Oct 2, 2017

Trott commented Oct 4, 2017

test: skip test if host is too slow #15688

test: skip test if host is too slow #15688

Conversation

Trott commented Sep 29, 2017

Checklist

Affected core subsystem(s)

Trott commented Sep 29, 2017

Trott commented Sep 29, 2017

Trott commented Sep 29, 2017

refack left a comment

Choose a reason for hiding this comment

refack Sep 29, 2017

Choose a reason for hiding this comment

refack Sep 29, 2017

Choose a reason for hiding this comment

refack Sep 29, 2017

Choose a reason for hiding this comment

Trott commented Sep 29, 2017

Fishrock123 commented Oct 2, 2017

refack commented Oct 2, 2017

Trott commented Oct 4, 2017