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: http outgoing _renderHeaders #13981

Closed
wants to merge 1 commit into from
Closed

Conversation

peteyycz
Copy link
Contributor

Improve http outgoing test coverage

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

http-outgoing

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 29, 2017
const common = require('../common');
const assert = require('assert');

const outHeadersKey = require('internal/http').outHeadersKey;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will probably need to have // Flags: --expose-internal added on the line below 'use strict';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is that automatically read by node, or is that for fellow developers? 😄

Copy link
Member

Choose a reason for hiding this comment

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

It’s read by the test runner, so if you run it as make test or python tools/test.py parallel/test-http-outgoing-renderHeaders, it will automatically be used. If you want to run the test file manually as ./node test/parallel/test-http-outgoing-renderHeaders.js, you’ll also need to add the flag yourself.

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Jun 29, 2017
const http = require('http');
const OutgoingMessage = http.OutgoingMessage;

const outgoingMessage = new OutgoingMessage();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you isolate these tests using block scoping. That prevents the need for outgoingMessage, outgoingMessage2, etc. It also reduces the potential for the tests interacting with each other unintentionally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed the code to use block scoping, it seemed a dirty solution without it, thanks for the advice!

@@ -0,0 +1,47 @@
'use strict'
// Flags: --expose-internal
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this pass make test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were right I missed a s at the end. Thanks.

@peteyycz
Copy link
Contributor Author

peteyycz commented Jul 8, 2017

is there anything I should do to get this merged?

@refack refack self-assigned this Jul 10, 2017
@refack
Copy link
Contributor

refack commented Jul 10, 2017

@Peteyy we need to run the automatic test suite before this could land. For technical reasons that will only be possible tomorrow (2017-07-10). I'm set myself a calendar reminder.

@peteyycz
Copy link
Contributor Author

@refack any news since then?

@jasnell
Copy link
Member

jasnell commented Jul 14, 2017

refack pushed a commit to refack/node that referenced this pull request Jul 14, 2017
PR-URL: nodejs#13981
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@refack
Copy link
Contributor

refack commented Jul 14, 2017

Landed in 074b7c2

@Peteyy congrats, GitHub just promoted you from
image
to
image
🍾

@refack refack closed this Jul 14, 2017
@addaleax addaleax mentioned this pull request Jul 18, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
PR-URL: #13981
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
PR-URL: #13981
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@refack refack removed their assignment Oct 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants