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.

Sign up for GitHub

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

Jump to bottom

pummel/test-tls-session-timeout failing #26839

Closed
Trott opened this issue Mar 21, 2019 · 7 comments
Closed

pummel/test-tls-session-timeout failing #26839

Trott opened this issue Mar 21, 2019 · 7 comments
Labels
confirmed-bug Issues with confirmed bugs. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.

Comments

@Trott
Copy link
Member

Trott commented Mar 21, 2019

https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/5519/console

test-rackspace-ubuntu1604-x64-1

00:06:39 not ok 101 pummel/test-tls-session-timeout
00:06:39   ---
00:06:39   duration_ms: 0.214
00:06:39   severity: fail
00:06:39   exitcode: 1
00:06:39   stack: |-
00:06:39     assert.js:85
00:06:39       throw new AssertionError(obj);
00:06:39       ^
00:06:39     
00:06:39     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
00:06:39     + actual - expected
00:06:39     
00:06:39     + 'New'
00:06:39     - 'Reused'
00:06:39         at /home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/test/pummel/test-tls-session-timeout.js:121:16
00:06:39         at ChildProcess.<anonymous> (/home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/test/pummel/test-tls-session-timeout.js:105:7)
00:06:39         at ChildProcess.emit (events.js:198:13)
00:06:39         at Process.ChildProcess._handle.onexit (internal/child_process.js:254:12)
00:06:39   ...
@Trott Trott added tls Issues and PRs related to the tls subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI. labels Mar 21, 2019
@Trott
Copy link
Member Author

Trott commented Mar 22, 2019

Two days in a row...

https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/5572/

test-rackspace-ubuntu1604-x64-1

00:07:01 not ok 101 pummel/test-tls-session-timeout
00:07:01   ---
00:07:01   duration_ms: 0.266
00:07:01   severity: fail
00:07:01   exitcode: 1
00:07:01   stack: |-
00:07:01     assert.js:87
00:07:01       throw new AssertionError(obj);
00:07:01       ^
00:07:01     
00:07:01     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
00:07:01     + actual - expected
00:07:01     
00:07:01     + 'New'
00:07:01     - 'Reused'
00:07:01         at /home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/test/pummel/test-tls-session-timeout.js:121:16
00:07:01         at ChildProcess.<anonymous> (/home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/test/pummel/test-tls-session-timeout.js:105:7)
00:07:01         at ChildProcess.emit (events.js:193:13)
00:07:01         at Process.ChildProcess._handle.onexit (internal/child_process.js:254:12)
00:07:01   ...

@Trott
Copy link
Member Author

Trott commented Mar 22, 2019

Running locally confirms this is not flaky but actually full-on broken. :-( Had to be something that landed very recently, so I'll bisect. Chances are the test just needs to be updated. Might turn out that it could/should be moved to sequential or parallel as well. More soon....

@Trott Trott changed the title Investigate flaky pummel/test-tls-session-timeout pummel/test-tls-session-timeout failing Mar 22, 2019
@Trott Trott added test Issues and PRs related to the tests. confirmed-bug Issues with confirmed bugs. and removed flaky-test Issues and PRs related to the tests with unstable failures on the CI. labels Mar 22, 2019
@Trott
Copy link
Member Author

Trott commented Mar 22, 2019

The first commit that causes this test to fail is 42dbaed. @sam-github Can you look at the test and the results and confirm whether the test needs to be adjusted or if the test failure is indicative of an actual bug introduced in that commit?

@Trott
Copy link
Member Author

Trott commented Mar 22, 2019

Here's the comment explaining what the test does:

// This test consists of three TLS requests --
// * The first one should result in a new connection because we don't have
//   a valid session ticket.
// * The second one should result in connection resumption because we used
//   the session ticket we saved from the first connection.
// * The third one should result in a new connection because the ticket
//   that we used has expired by now.

The failure seems to be with the second connection. It is detecting that it is a new connection when it is expecting it to be a reused connection.

@Trott
Copy link
Member Author

Trott commented Mar 22, 2019

I "fixed" it by setting maxVersion to 'TLSv1.2'. Hopefully that's the correct approach. PR coming momentarily.

Trott added a commit to Trott/io.js that referenced this issue Mar 22, 2019
@Trott
test: fix pummel/test-tls-session-timeout
3ea61d3 
The test does not work with TLS 1.3 nor should it. Force TLS version
1.2.

While at it, some refactoring:

* refresh the tmp directory in case it doesn't exist!
* add an assert.strictEqual() check on the client return `code` value
  which must be zero
* use arrow functions for callbacks
* add trailing commas for multiline arrays/objects

Fixes: nodejs#26839
@Trott
Copy link
Member Author

Trott commented Mar 22, 2019

PR to address this: #26865

@Trott Trott closed this as completed in cc89e68 Mar 22, 2019
targos pushed a commit to targos/node that referenced this issue Mar 27, 2019
@Trott @targos
test: fix pummel/test-tls-session-timeout
ac6e297 
The test does not work with TLS 1.3 nor should it. Force TLS version
1.2.

While at it, some refactoring:

* refresh the tmp directory in case it doesn't exist!
* add an assert.strictEqual() check on the client return `code` value
  which must be zero
* use arrow functions for callbacks
* add trailing commas for multiline arrays/objects

Fixes: nodejs#26839

PR-URL: nodejs#26865
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this issue Mar 27, 2019
@Trott @targos
test: fix pummel/test-tls-session-timeout
8ca7d56 
The test does not work with TLS 1.3 nor should it. Force TLS version
1.2.

While at it, some refactoring:

* refresh the tmp directory in case it doesn't exist!
* add an assert.strictEqual() check on the client return `code` value
  which must be zero
* use arrow functions for callbacks
* add trailing commas for multiline arrays/objects

Fixes: #26839

PR-URL: #26865
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@richardlau
Copy link
Member

Seen again in https://ci.nodejs.org/job/node-test-binary-windows/25187/COMPILED_BY=vs2017,RUNNER=win10,RUN_SUBSET=1/testReport/junit/(root)/test/sequential_test_tls_session_timeout/

15:11:36 not ok 646 sequential/test-tls-session-timeout
15:11:36   ---
15:11:36   duration_ms: 4.333
15:11:36   severity: fail
15:11:36   exitcode: 1
15:11:36   stack: |-
15:11:36     assert.js:87
15:11:36       throw new AssertionError(obj);
15:11:36       ^
15:11:36     
15:11:36     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
15:11:36     + actual - expected
15:11:36     
15:11:36     + 'New'
15:11:36     - 'Reused'
15:11:36         at c:\workspace\node-test-binary-windows\test\sequential\test-tls-session-timeout.js:124:16
15:11:36         at ChildProcess.<anonymous> (c:\workspace\node-test-binary-windows\test\sequential\test-tls-session-timeout.js:108:7)
15:11:36         at ChildProcess.emit (events.js:196:13)
15:11:36         at Process.ChildProcess._handle.onexit (internal/child_process.js:256:12)
15:11:36   ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
confirmed-bug Issues with confirmed bugs. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Trott @richardlau