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

test: fix race condition in connection-handshake #260

Closed
wants to merge 1 commit into from
Closed

test: fix race condition in connection-handshake #260

wants to merge 1 commit into from

Conversation

nechaido
Copy link
Member

No description provided.

@nechaido nechaido added the test label Jul 21, 2017
@nechaido nechaido requested a review from aqrln July 21, 2017 09:12
aqrln
aqrln approved these changes Jul 21, 2017
View reviewed changes
test/node/connection-handshake.js Outdated
test.pass('connection must be closed');
});
const connection = net.connect(port, error =>
test.assertNot(error, 'must connect to server')
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this function should have braces.

@aqrln
Copy link
Member

aqrln commented Jul 21, 2017 •
edited
Loading

Strictly speaking, this doesn't have anything to do with macOS per se, it's just the bug hasn't been reproducing on Linux, so perhaps something like "test: fix race condition in connection-handshake" might be a better commit message.

Aside from that, the fact that it was only failing on macOS consistently makes it apparent that we need better platform coverage in CI, i.e., we should add macOS to the Travis configuration matrix and start using AppVeyor to test code on Windows.

@nechaido nechaido force-pushed the test-fix-handshake-timeout branch from 426d675 to 7122770 Compare July 21, 2017 09:21
@nechaido nechaido force-pushed the test-fix-handshake-timeout branch from 7122770 to 709ee7d Compare July 21, 2017 09:22
@nechaido nechaido changed the title test: fix handshake timeout test failing on macOS est: fix race condition in connection-handshake Jul 21, 2017
@nechaido nechaido changed the title est: fix race condition in connection-handshake test: fix race condition in connection-handshake Jul 21, 2017
@nechaido nechaido added this to the 1.0.0 milestone Jul 21, 2017
belochub
belochub approved these changes Jul 21, 2017
View reviewed changes
Copy link
Member

@belochub belochub left a comment

Choose a reason for hiding this comment

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

LGTM

@aqrln aqrln mentioned this pull request Jul 22, 2017
Closed
aqrln added a commit that referenced this pull request Jul 22, 2017
@aqrln
test: rewrite the Travis config
2c1797d 
* Use only latests releases of Node.js 6.x, 7.x and 8.x.
* Update Ubuntu from Precise to Trusty.
* Don't install a custom compiler, Trusty already has one that should
  support C++11.
* Run tests on macOS too.
* Don't install Firefox and don't launch the fake X server since we
  don't currently have browser tests anymore.  When we do, it will
  require to rewrite that part of the config in a cross-platform manner.

Fixes: #262
Refs: #260 (comment)
@aqrln aqrln mentioned this pull request Jul 22, 2017
Closed
@aqrln
Copy link
Member

aqrln commented Jul 22, 2017

I'd want to fast-track this fix. Without the patch, tests are currently failing on master for me.

belochub pushed a commit that referenced this pull request Jul 22, 2017
@nechaido @belochub
test: fix race condition in connection-handshake
90c064c 
PR-URL: #260
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@belochub
Copy link
Member

Landed in 90c064c.

@belochub belochub closed this Jul 22, 2017
@belochub belochub deleted the test-fix-handshake-timeout branch July 22, 2017 09:49
aqrln added a commit that referenced this pull request Jul 22, 2017
@aqrln
test: rewrite the Travis config
6e544f4 
* Use only latests releases of Node.js 6.x, 7.x and 8.x.
* Update Ubuntu from Precise to Trusty.
* Don't install a custom compiler, Trusty already has one that should
  support C++11.
* Run tests on macOS too.
* Don't install Firefox and don't launch the fake X server since we
  don't currently have browser tests anymore.  When we do, it will
  require to rewrite that part of the config in a cross-platform manner.

Fixes: #262
Refs: #260 (comment)
aqrln added a commit that referenced this pull request Jul 22, 2017
@aqrln
test: rewrite the Travis config
5eb6c7b 
* Use only latests releases of Node.js 6.x, 7.x and 8.x.
* Update Ubuntu from Precise to Trusty.
* Don't install a custom compiler, Trusty already has one that should
  support C++11.
* Run tests on macOS too.
* Don't install Firefox and don't launch the fake X server since we
  don't currently have browser tests anymore.  When we do, it will
  require to rewrite that part of the config in a cross-platform manner.

Fixes: #262
Refs: #260 (comment)
aqrln added a commit that referenced this pull request Jul 24, 2017
@aqrln
test: rewrite the Travis config
bb5273f 
* Use only latests releases of Node.js 6.x, 7.x and 8.x.
* Update Ubuntu from Precise to Trusty.
* Don't install a custom compiler, Trusty already has one that should
  support C++11.
* Run tests on macOS too.
* Don't install Firefox and don't launch the fake X server since we
  don't currently have browser tests anymore.  When we do, it will
  require to rewrite that part of the config in a cross-platform manner.

Fixes: #262
Refs: #260 (comment)
aqrln added a commit that referenced this pull request Jul 24, 2017
@aqrln
test: rewrite the Travis config
87947ee 
* Use only latests releases of Node.js 6.x, 7.x and 8.x.
* Update Ubuntu from Precise to Trusty.
* Don't install a custom compiler, Trusty already has one that should
  support C++11.
* Run tests on macOS too.
* Don't install Firefox and don't launch the fake X server since we
  don't currently have browser tests anymore.  When we do, it will
  require to rewrite that part of the config in a cross-platform manner.

Fixes: #262
Refs: #260 (comment)
PR-URL: #263
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
nechaido pushed a commit that referenced this pull request Jul 25, 2017
@aqrln @nechaido
test: rewrite the Travis config
ec17b9d 
* Use only latests releases of Node.js 6.x, 7.x and 8.x.
* Update Ubuntu from Precise to Trusty.
* Don't install a custom compiler, Trusty already has one that should
  support C++11.
* Run tests on macOS too.
* Don't install Firefox and don't launch the fake X server since we
  don't currently have browser tests anymore.  When we do, it will
  require to rewrite that part of the config in a cross-platform manner.

Fixes: #262
Refs: #260 (comment)
PR-URL: #263
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
aqrln added a commit that referenced this pull request Jul 26, 2017
@aqrln
test: add .appveyor.yml
42356f3 
Refs: #260 (comment)
PR-URL: #261
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
belochub pushed a commit that referenced this pull request Jan 22, 2018
@nechaido @belochub
test: fix race condition in connection-handshake
c695eae 
PR-URL: #260
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
belochub pushed a commit that referenced this pull request Jan 22, 2018
@aqrln @belochub
test: rewrite the Travis config
f966833 
* Use only latests releases of Node.js 6.x, 7.x and 8.x.
* Update Ubuntu from Precise to Trusty.
* Don't install a custom compiler, Trusty already has one that should
  support C++11.
* Run tests on macOS too.
* Don't install Firefox and don't launch the fake X server since we
  don't currently have browser tests anymore.  When we do, it will
  require to rewrite that part of the config in a cross-platform manner.

Fixes: #262
Refs: #260 (comment)
PR-URL: #263
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
belochub pushed a commit that referenced this pull request Jan 22, 2018
@aqrln @belochub
test: add .appveyor.yml
273ff5e 
Refs: #260 (comment)
PR-URL: #261
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
belochub pushed a commit that referenced this pull request Jan 22, 2018
@nechaido @belochub
test: fix race condition in connection-handshake
8130a0e 
PR-URL: #260
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
belochub pushed a commit that referenced this pull request Jan 22, 2018
@aqrln @belochub
test: rewrite the Travis config
15311e0 
* Use only latests releases of Node.js 6.x, 7.x and 8.x.
* Update Ubuntu from Precise to Trusty.
* Don't install a custom compiler, Trusty already has one that should
  support C++11.
* Run tests on macOS too.
* Don't install Firefox and don't launch the fake X server since we
  don't currently have browser tests anymore.  When we do, it will
  require to rewrite that part of the config in a cross-platform manner.

Fixes: #262
Refs: #260 (comment)
PR-URL: #263
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
belochub pushed a commit that referenced this pull request Jan 22, 2018
@aqrln @belochub
test: add .appveyor.yml
3af5017 
Refs: #260 (comment)
PR-URL: #261
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
@belochub belochub mentioned this pull request Jan 22, 2018
Merged
belochub pushed a commit to metarhia/mdsf that referenced this pull request Jul 19, 2018
@aqrln
test: rewrite the Travis config
183cc96 
* Use only latests releases of Node.js 6.x, 7.x and 8.x.
* Update Ubuntu from Precise to Trusty.
* Don't install a custom compiler, Trusty already has one that should
  support C++11.
* Run tests on macOS too.
* Don't install Firefox and don't launch the fake X server since we
  don't currently have browser tests anymore.  When we do, it will
  require to rewrite that part of the config in a cross-platform manner.

Fixes: metarhia/jstp#262
Refs: metarhia/jstp#260 (comment)
PR-URL: metarhia/jstp#263
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
belochub pushed a commit to metarhia/mdsf that referenced this pull request Jul 19, 2018
@aqrln
test: add .appveyor.yml
c091018 
Refs: metarhia/jstp#260 (comment)
PR-URL: metarhia/jstp#261
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
belochub pushed a commit to metarhia/mdsf that referenced this pull request Jul 19, 2018
@aqrln
test: rewrite the Travis config
1b544bb 
* Use only latests releases of Node.js 6.x, 7.x and 8.x.
* Update Ubuntu from Precise to Trusty.
* Don't install a custom compiler, Trusty already has one that should
  support C++11.
* Run tests on macOS too.
* Don't install Firefox and don't launch the fake X server since we
  don't currently have browser tests anymore.  When we do, it will
  require to rewrite that part of the config in a cross-platform manner.

Fixes: metarhia/jstp#262
Refs: metarhia/jstp#260 (comment)
PR-URL: metarhia/jstp#263
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
belochub pushed a commit to metarhia/mdsf that referenced this pull request Jul 19, 2018
@aqrln
test: add .appveyor.yml
e3065f6 
Refs: metarhia/jstp#260 (comment)
PR-URL: metarhia/jstp#261
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
belochub pushed a commit to metarhia/mdsf that referenced this pull request Jul 21, 2018
@aqrln @belochub
test: rewrite the Travis config
9353726 
* Use only latests releases of Node.js 6.x, 7.x and 8.x.
* Update Ubuntu from Precise to Trusty.
* Don't install a custom compiler, Trusty already has one that should
  support C++11.
* Run tests on macOS too.
* Don't install Firefox and don't launch the fake X server since we
  don't currently have browser tests anymore.  When we do, it will
  require to rewrite that part of the config in a cross-platform manner.

Fixes: metarhia/jstp#262
Refs: metarhia/jstp#260 (comment)
PR-URL: metarhia/jstp#263
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
belochub pushed a commit to metarhia/mdsf that referenced this pull request Jul 21, 2018
@aqrln @belochub
test: add .appveyor.yml
9cc85b1 
Refs: metarhia/jstp#260 (comment)
PR-URL: metarhia/jstp#261
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@belochub belochub belochub approved these changes

@aqrln aqrln aqrln approved these changes

@lundibundi lundibundi lundibundi approved these changes

Assignees
No one assigned
Labels
macos test windows
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@nechaido @aqrln @belochub @lundibundi