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

test,async_hooks: stabilize tests on Windows #13381

Merged
merged 2 commits into from
Jul 2, 2017

Conversation

refack
Copy link
Contributor

@refack refack commented Jun 1, 2017

new

  • test-emit-before-after
=== release test-fseventwrap ===
Path: async-hooks/test-fseventwrap
assert.js:60
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: Checking invocations at stage "when process exits":
Never called "destroy", but expected 1 invocation(s).
at checkHook (D:\code\node\test\async-hooks\hook-checks.js:45:7)
at Array.forEach (native)
at checkInvocations (D:\code\node\test\async-hooks\hook-checks.js:28:44)
at process.onexit (D:\code\node\test\async-hooks\test-fseventwrap.js:32:3)
at emitOne (events.js:120:20)
at process.emit (events.js:210:7)
Command: D:\code\node\Release\node.exe D:\code\node\test\async-hooks\test-fseventwrap.js
=== release test-graph.signal ===
Path: async-hooks/test-graph.signal
Mismatched onsigusr2 function calls. Expected exactly 2, actual 0.
at Object.exports.mustCall (D:\code\node\test\common\index.js:483:10)
at Object. (D:\code\node\test\async-hooks\test-graph.signal.js:11:30)
at Module._compile (module.js:569:30)
at Object.Module._extensions..js (module.js:580:10)
at Module.load (module.js:503:32)
at tryModuleLoad (module.js:466:12)
at Function.Module._load (module.js:458:3)
at Function.Module.runMain (module.js:605:10)
at startup (bootstrap_node.js:158:16)
Command: D:\code\node\Release\node.exe D:\code\node\test\async-hooks\test-graph.signal.js
=== release test-pipeconnectwrap ===
Path: async-hooks/test-pipeconnectwrap
assert.js:60
throw new errors.AssertionError({
^

AssertionError [ERR_ASSERTION]: Checking invocations at stage "pipe2, process exiting":
Never called "destroy", but expected 1 invocation(s).
at checkHook (D:\code\node\test\async-hooks\hook-checks.js:45:7)
at Array.forEach (native)
at checkInvocations (D:\code\node\test\async-hooks\hook-checks.js:28:44)
at process.onexit (D:\code\node\test\async-hooks\test-pipeconnectwrap.js:98:3)
at emitOne (events.js:120:20)
at process.emit (events.js:210:7)
Command: D:\code\node\Release\node.exe D:\code\node\test\async-hooks\test-pipeconnectwrap.js
=== release test-signalwrap ===
Path: async-hooks/test-signalwrap
assert.js:60
throw new errors.AssertionError({
^

AssertionError [ERR_ASSERTION]: one signal wrap when SIGUSR2 handler is set up
at Object. (D:\code\node\test\async-hooks\test-signalwrap.js:15:8)
at Module._compile (module.js:569:30)
at Object.Module._extensions..js (module.js:580:10)
at Module.load (module.js:503:32)
at tryModuleLoad (module.js:466:12)
at Function.Module._load (module.js:458:3)
at Function.Module.runMain (module.js:605:10)
at startup (bootstrap_node.js:158:16)
at bootstrap_node.js:575:3
Command: D:\code\node\Release\node.exe D:\code\node\test\async-hooks\test-signalwrap.js
=== release test-ttywrap.readstream ===
Path: async-hooks/test-ttywrap.readstream
assert.js:60
throw new errors.AssertionError({
^

AssertionError [ERR_ASSERTION]: Checking invocations at stage "when tty ended ":
Never called "before", but expected 1 invocation(s).
at checkHook (D:\code\node\test\async-hooks\hook-checks.js:45:7)
at Array.forEach (native)
at checkInvocations (D:\code\node\test\async-hooks\hook-checks.js:28:44)
at common.mustCall (D:\code\node\test\async-hooks\test-ttywrap.readstream.js:29:5)
at D:\code\node\test\common\index.js:517:15
at Immediate.ontick (D:\code\node\test\async-hooks\tick.js:7:37)
at runCallback (timers.js:800:20)
at tryOnImmediate (timers.js:762:5)
at processImmediate [as _immediateCallback] (timers.js:733:5)
Command: D:\code\node\Release\node.exe D:\code\node\test\async-hooks\test-ttywrap.readstream.js
=== release test-writewrap ===
Path: async-hooks/test-writewrap
assert.js:60
throw new errors.AssertionError({
^

AssertionError [ERR_ASSERTION]: Checking invocations at stage "when server got secure connection":
Never called "destroy", but expected 1 invocation(s).
at checkHook (D:\code\node\test\async-hooks\hook-checks.js:45:7)
at Array.forEach (native)
at checkInvocations (D:\code\node\test\async-hooks\hook-checks.js:28:44)
at checkValidWriteWrap (D:\code\node\test\async-hooks\test-writewrap.js:56:5)
at Array.forEach (native)
at checkDestroyedWriteWraps (D:\code\node\test\async-hooks\test-writewrap.js:58:6)
at Server.onsecureConnection (D:\code\node\test\async-hooks\test-writewrap.js:65:3)
at Server. (D:\code\node\test\common\index.js:517:15)
at emitOne (events.js:115:13)
at Server.emit (events.js:210:7)
Command: D:\code\node\Release\node.exe D:\code\node\test\async-hooks\test-writewrap.js
[00:16|% 100|+ 39|- 6]: Done

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test,async_hooks

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels Jun 1, 2017
@refack refack added the wip Issues and PRs that are still a work in progress. label Jun 1, 2017
@refack refack self-assigned this Jun 1, 2017
@refack refack added the windows Issues and PRs related to the Windows platform. label Jun 1, 2017
@@ -1,6 +1,11 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit curious about the name of this file... test-graph.signal.js ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@AndreasMadsen AndreasMadsen Jun 2, 2017

Choose a reason for hiding this comment

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

Most of the async hooks can be represented as a graph/tree (see https://dprof.js.org). The test-graph.*.js files check that the graph is correct. The test-graph.signal.js file check that the graph regarding signals is correct.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I get the purpose of the test, just not why it doesn't follow the naming pattern of other tests (specifically, why test-graph.signal instead of test-graph-signal)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is any reason, we can rename.

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Jun 2, 2017

@refack Thanks for looking into these. Unfortunately I can't really help much, but I can give some thoughts and guesses, hopefully, it helps.

  • test\async-hooks\test-fseventwrap.js: looks like a GC sensitive test. tick(2, fn) seams very suspicious.

  • test\async-hooks\test-pipeconnectwrap.js: again GC issue, tick(5) is suspicious.

  • test/async-hooks/test-signalwrap.js: also a signal on windows issue, if I'm not mistaken.

  • async-hooks/test-ttywrap.readstream.js: again GC issue, tick(2, fn) is suspicious.

  • async-hooks\test-writewrap.js: could be a "race condition" (sort of) between the server onconnection and client onconnect. We have seen this being OS dependent in another test case.

To summarize, I don't think there are any actual bugs the tests are just not robust enough.

@refack
Copy link
Contributor Author

refack commented Jun 9, 2017

Post #13336 run:

=== release test-emit-before-after ===
Path: async-hooks/test-emit-before-after
assert.js:60
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 'Error: before(): asyncId or triggerId is less than zero (asyncId: -1, triggerId: -1)\r' === 'Error: before(): asyncId or triggerId is le
ss than zero (asyncId: -1, triggerId: -1)'
at Object. (D:\code\node\test\async-hooks\test-emit-before-after.js:19:8)
at Module._compile (module.js:569:30)
at Object.Module._extensions..js (module.js:580:10)
at Module.load (module.js:503:32)
at tryModuleLoad (module.js:466:12)
at Function.Module._load (module.js:458:3)
at Function.Module.runMain (module.js:605:10)
at startup (bootstrap_node.js:158:16)
at bootstrap_node.js:575:3
Command: D:\code\node\Release\node.exe D:\code\node\test\async-hooks\test-emit-before-after.js
=== release test-graph.signal ===
Path: async-hooks/test-graph.signal
Mismatched onsigusr2 function calls. Expected exactly 2, actual 0.
at Object.exports.mustCall (D:\code\node\test\common\index.js:483:10)
at Object. (D:\code\node\test\async-hooks\test-graph.signal.js:11:30)
at Module._compile (module.js:569:30)
at Object.Module._extensions..js (module.js:580:10)
at Module.load (module.js:503:32)
at tryModuleLoad (module.js:466:12)
at Function.Module._load (module.js:458:3)
at Function.Module.runMain (module.js:605:10)
at startup (bootstrap_node.js:158:16)
Command: D:\code\node\Release\node.exe D:\code\node\test\async-hooks\test-graph.signal.js
=== release test-signalwrap ===
Path: async-hooks/test-signalwrap
assert.js:60
throw new errors.AssertionError({
^

AssertionError [ERR_ASSERTION]: 0 === 1
at Object. (D:\code\node\test\async-hooks\test-signalwrap.js:15:8)
at Module._compile (module.js:569:30)
at Object.Module._extensions..js (module.js:580:10)
at Module.load (module.js:503:32)
at tryModuleLoad (module.js:466:12)
at Function.Module._load (module.js:458:3)
at Function.Module.runMain (module.js:605:10)
at startup (bootstrap_node.js:158:16)
at bootstrap_node.js:575:3
Command: D:\code\node\Release\node.exe D:\code\node\test\async-hooks\test-signalwrap.js
=== release test-ttywrap.readstream ===
Path: async-hooks/test-ttywrap.readstream
assert.js:60
throw new errors.AssertionError({
^

AssertionError [ERR_ASSERTION]: Checking invocations at stage "when tty ended ":
Never called "before", but expected 1 invocation(s).
at checkHook (D:\code\node\test\async-hooks\hook-checks.js:45:7)
at Array.forEach ()
at checkInvocations (D:\code\node\test\async-hooks\hook-checks.js:28:44)
at common.mustCall (D:\code\node\test\async-hooks\test-ttywrap.readstream.js:29:5)
at D:\code\node\test\common\index.js:517:15
at Immediate.ontick (D:\code\node\test\async-hooks\tick.js:7:37)
at runCallback (timers.js:800:20)
at tryOnImmediate (timers.js:762:5)
at processImmediate [as _immediateCallback] (timers.js:733:5)
Command: D:\code\node\Release\node.exe D:\code\node\test\async-hooks\test-ttywrap.readstream.js

@refack refack force-pushed the stabalize-async-tests-windows branch from 52583d0 to b0f0119 Compare June 10, 2017 14:13
@refack refack removed the wip Issues and PRs that are still a work in progress. label Jun 10, 2017
@refack
Copy link
Contributor Author

refack commented Jun 10, 2017

@refack
Copy link
Contributor Author

refack commented Jun 10, 2017

@refack
Copy link
Contributor Author

refack commented Jun 10, 2017

@refack refack changed the title [WIP] test,async_hooks: stabilize tests on Windows test,async_hooks: stabilize tests on Windows Jun 10, 2017
@refack
Copy link
Contributor Author

refack commented Jun 10, 2017

@DavidCai1993 @nodejs/async_hooks PTAL at b0f0119

@refack refack force-pushed the stabalize-async-tests-windows branch from 5d009c8 to f83c32d Compare June 10, 2017 17:27
@refack
Copy link
Contributor Author

refack commented Jun 10, 2017

@refack
Copy link
Contributor Author

refack commented Jun 10, 2017

reported flake #13603
rerun arm: https://ci.nodejs.org/job/node-test-commit-arm/10259/

@AndreasMadsen
Copy link
Member

@refack Could you rebase this? Then I will take a closer look.

@refack refack force-pushed the stabalize-async-tests-windows branch from f83c32d to 8feb68c Compare July 1, 2017 22:17
@refack
Copy link
Contributor Author

refack commented Jul 1, 2017

Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

LGTM, assuming the CI is green on all platforms.

Thanks for taking care of this, it is was a bit embarrassing that we didn't have tests enabled for windows.

@refack
Copy link
Contributor Author

refack commented Jul 2, 2017

@refack
Copy link
Contributor Author

refack commented Jul 2, 2017

Arm fail is known: #14015

PR-URL: nodejs#13381
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
PR-URL: nodejs#13381
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
@refack refack force-pushed the stabalize-async-tests-windows branch from 8feb68c to 7022260 Compare July 2, 2017 02:05
@refack refack merged commit 7022260 into nodejs:master Jul 2, 2017
@refack
Copy link
Contributor Author

refack commented Jul 2, 2017

Landed in 372b85d, 7022260

@refack refack deleted the stabalize-async-tests-windows branch July 2, 2017 02:33
addaleax pushed a commit to addaleax/node that referenced this pull request Jul 3, 2017
PR-URL: nodejs#13381
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
addaleax pushed a commit to addaleax/node that referenced this pull request Jul 3, 2017
PR-URL: nodejs#13381
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
@addaleax addaleax mentioned this pull request Jul 3, 2017
addaleax pushed a commit that referenced this pull request Jul 11, 2017
PR-URL: #13381
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
PR-URL: #13381
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
PR-URL: #13381
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
PR-URL: #13381
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
@refack refack removed their assignment Oct 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants