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: refactor test-http-agent-timeout-option #25854

Closed
wants to merge 1 commit into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Jan 31, 2019

Fix flakyness caused by usage of a non-routable IP address.

Refs: #25488 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Fix flakyness caused by usage of a non-routable IP address.

Refs: nodejs#25488 (comment)
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 31, 2019
@lpinca
Copy link
Member Author

lpinca commented Jan 31, 2019

@Trott
Copy link
Member

Trott commented Feb 2, 2019

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/20533/ 🤘

@Trott
Copy link
Member

Trott commented Feb 2, 2019

Would like to fast-track this since it's fixing a flaky test. (Just got it myself in https://ci.nodejs.org/job/node-test-binary-arm/5905/RUN_SUBSET=4,label=pi2-docker/console.)

Please 👍 here to fast-track.

@Trott Trott added fast-track PRs that do not need to wait for 48 hours to land. flaky-test Issues and PRs related to the tests with unstable failures on the CI. labels Feb 2, 2019
@Trott
Copy link
Member

Trott commented Feb 2, 2019

Landed in 988482e

@Trott Trott closed this Feb 2, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Feb 2, 2019
Fix flakyness caused by usage of a non-routable IP address.

Refs: nodejs#25488 (comment)

PR-URL: nodejs#25854
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@lpinca lpinca deleted the fix/flaky-test branch February 2, 2019 06:50
@lpinca
Copy link
Member Author

lpinca commented Feb 2, 2019

Was writing a simpler version without establishing a TCP connection but I did not make it in time.

'use strict';

const { mustCall } = require('../common');
const { strictEqual } = require('assert');
const { Agent, get } = require('http');

// Test that the listener that forwards the `'timeout'` event from the socket to
// the `ClientRequest` instance is added to the socket when the `timeout` option
// of the `Agent` is set.

const request = get({
  agent: new Agent({ timeout: 50 }),
  lookup: () => {}
});

request.on('socket', mustCall((socket) => {
  strictEqual(socket.timeout, 50);

  const listeners = socket.listeners('timeout');

  strictEqual(listeners.length, 1);
  strictEqual(listeners[0], request.timeoutCb);
}));

@Trott @gireeshpunathil what you think, does it worth to effort to refactor it again?

@gireeshpunathil
Copy link
Member

this looks good, but given the premise is CI stability and that is established with the one that is landed may be leave it at that, visit again if it flakes?

@lpinca
Copy link
Member Author

lpinca commented Feb 2, 2019

Yes, the only reason was to make it faster, no need to wait 500 ms or whatever the the timeout is, it should only test that the listener is added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. flaky-test Issues and PRs related to the tests with unstable failures on the CI. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants