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: fix test-debug-signal-cluster.js flakyness #8568

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@misterdjules
Contributor

misterdjules commented Sep 16, 2016

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

Tests.

Description of change

Do not assume any order and buffering/atomicity of output from child
processes' debugger agents.

Fixes #3796.

Julien Gilli
test: fix test-debug-signal-cluster.js flakyness
Do not assume any order and buffering/atomicity of output from child
processes' debugger agents.

Fixes #3796.
@misterdjules

This comment has been minimized.

Contributor

misterdjules commented Sep 16, 2016

assert.deepStrictEqual(outputLines, expectedLines);
assert.equal(expectedContent, '');

This comment has been minimized.

@Trott

Trott Sep 16, 2016

Member

Nit: assert.strictEqual() (not necessary, just a preference, hence the Nit, ignore if you have strong contrary feelings)

This comment has been minimized.

@misterdjules

misterdjules Sep 16, 2016

Contributor

Good catch, thanks!

@Trott

Trott approved these changes Sep 16, 2016

LGTM if CI and stress tests come up clean

// output may be interleaved arbitrarily. Moreover, child processes' output
// may be written using an arbitrary number of system calls, and no assumption
// on buffering or atomicity of output should be made. Thus, we process the
// output of all chid processes' debugger agents character by character, and

This comment has been minimized.

@imyller

imyller Sep 16, 2016

Member

nit: spotted a comment typo

This comment has been minimized.

@misterdjules

misterdjules Sep 16, 2016

Contributor

Good catch, thanks!

@imyller

LGTM pending CI and stress test

@misterdjules

This comment has been minimized.

Contributor

misterdjules commented Sep 16, 2016

Rerunning tests on FreeBSD since tests failed to start on that platform during the previous CI tests run.

@misterdjules

This comment has been minimized.

Contributor

misterdjules commented Sep 16, 2016

Updated PR according to code reviews with one separate commit: I'll squash all commits in this PR into one when/if it lands.

Tests on FreeBSD all passed, still waiting for stress tests to complete.

@imyller

LGTM with changes and green CI

@misterdjules

This comment has been minimized.

Contributor

misterdjules commented Sep 16, 2016

Pushed another commit that actually removes the test from the list of flaky tests. My apologies for omitting that from the first commit. We shouldn't need to rerun CI tests though, as they all turned green and we're running specific stress tests on the two platforms for which we're changing the flaky tests list.

@imyller

This comment has been minimized.

Member

imyller commented Sep 16, 2016

Agreed. CI clean (with usual unrelated failures) and stress test looks very promising

@misterdjules

This comment has been minimized.

Contributor

misterdjules commented Sep 16, 2016

Stress tests all passed, will land asap.

misterdjules pushed a commit that referenced this pull request Sep 16, 2016

Julien Gilli
test: fix test-debug-signal-cluster.js flakyness
Do not assume any order and buffering/atomicity of output from child
processes' debugger agents.

Fixes: #3796
PR-URL: #8568
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@misterdjules

This comment has been minimized.

Contributor

misterdjules commented Sep 16, 2016

Thank you @Trott and @imyller, landed in 7926886.

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Oct 10, 2016

Does not land cleanly on v6, attempting to patch maked the tests fail.

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete

@misterdjules misterdjules deleted the misterdjules:fix-issue-3797 branch Jul 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment