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

investigate flaky test-tls-regr-gh-5108 #10966

Closed
Trott opened this issue Jan 23, 2017 · 10 comments
Closed

investigate flaky test-tls-regr-gh-5108 #10966

Trott opened this issue Jan 23, 2017 · 10 comments
Labels
test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.

Comments

@Trott
Copy link
Member

Trott commented Jan 23, 2017

  • Version: v8.0.0-pre
  • Platform: smartos16-64
  • Subsystem: tls

https://ci.nodejs.org/job/node-test-commit-smartos/6523/nodes=smartos16-64/consoleFull:

not ok 1230 parallel/test-tls-regr-gh-5108
  ---
  duration_ms: 60.94
  severity: fail
  stack: |-
    timeout
@Trott Trott added test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem. labels Jan 23, 2017
@Trott
Copy link
Member Author

Trott commented Jan 23, 2017

Refs: aa05269
Refs: #5108

/cc @indutny @ChALkeR for any input/insight but not a lot of data to go on right now...

@Trott
Copy link
Member Author

Trott commented Jan 23, 2017

(Aside: This is a really good example of why I think it's good to have console.log() and/or console.error() statements in tests sometimes. I know it's contrary to the philosophy of some folks, but in a case like this where the test times out, it's helpful to be able to narrow down where the problem might be. And tests timing out with no info like this seems to be an increasing share of our flaky tests.)

@joyeecheung
Copy link
Member

FWIW I think doing console.error only when some environment variables or something in common are present for debugging purposes would be more acceptable to people who believe it shouldn't be used in tests? (don't really have a strong opinion about it myself)

@gibfahn
Copy link
Member

gibfahn commented Jan 23, 2017

+1 to console.log/console.error being used (sparingly) in tests, especially console.log, as it currently is only displayed if the test fails, so it doesn't clutter up normal test output.

@evanlucas
Copy link
Contributor

If we don't want to normally see them, why not add a util.debuglog to common and use something like common.log()?

@gibfahn
Copy link
Member

gibfahn commented Jan 25, 2017

@evanlucas how would you then allow them to be seen?

@evanlucas
Copy link
Contributor

@gibfahn NODE_DEBUG=test ./node test/...

@gibfahn
Copy link
Member

gibfahn commented Jan 25, 2017

@evanlucas Cool, I didn't know about that. Definite +1 on having a common.log().

Doc link: https://nodejs.org/api/util.html#util_util_debuglog_section

@misterdjules
Copy link

To paraphrase what I wrote in #11026:

So far, I haven't been able to reproduce the problem described by any of the issues listed above.

In order to be able to get more information and investigate future spurious failures, I submitted a PR that sends SIGABRT instead of SIGTERM to test processes that timeout. This will allow us to take a look at core files generated from these processes with tools such as llnode and mdb_v8, and will potentially help us root cause these issues.

In the meantime I'll continue trying to reproduce and investigate those issues, I'll keep you posted.

@Trott
Copy link
Member Author

Trott commented Jul 16, 2017

Haven't seen this in a while. Closing. Can re-open if it recurs.

@Trott Trott closed this as completed Jul 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants