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: improve http-client coverage #33633

Merged
merged 6 commits into from
Dec 19, 2023

Conversation

KuthorX
Copy link
Contributor

@KuthorX KuthorX commented May 29, 2020

Based on https://coverage.nodejs.org/coverage-d12d5ef3ef8e5d9f/lib/_http_client.js.html, I try to improve coverage to line 120 (http-client-input-function), 194-197 (http-client-insecure-http-parser-error)

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 29, 2020
assert.throws(() => {
new ClientRequest({ insecureHTTPParser: 'wrongValue' });
}, {
code: /ERR_INVALID_ARG_TYPE/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
code: /ERR_INVALID_ARG_TYPE/
code: 'ERR_INVALID_ARG_TYPE',
message: /insecureHTTPParser/

const ClientRequest = require('http').ClientRequest;

{
const req = new ClientRequest(() => {});
Copy link
Member

Choose a reason for hiding this comment

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

The test should definitely hit the mentioned code line but it would be great to write a test that actually verifies that the callback is also called and ideally, this test case would not cause any errors, since it's a legit code path.

Copy link
Contributor Author

@KuthorX KuthorX May 30, 2020

Choose a reason for hiding this comment

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

@BridgeAR Here is a rewrote version, which can make cb be called, but it's a little longer than previous version, so I am not sure whether it's ok, could you help me to check?

'use strict';

const common = require('../common');
const http = require('http');
const assert = require('assert');
const ClientRequest = require('http').ClientRequest;

{
  const server = http.createServer(common.mustCall((req, res) => {
    res.writeHead(200);
    res.end('hello world');
  })).listen(80, '127.0.0.1');
  
  const req = new ClientRequest(common.mustCall((response) => {
    let body = '';
    response.setEncoding('utf8');
    response.on('data', (chunk) => {
      body += chunk;
    });

    response.on('end', common.mustCall(() => {
      assert.strictEqual(body, 'hello world');
      server.close();
    }));
  }));

  req.end();
}

@BridgeAR
Copy link
Member

@KuthorX thank you very much for your contribution! This is already looking pretty good, just left two minor comments.

@KuthorX KuthorX requested a review from BridgeAR May 30, 2020 09:37
@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@KuthorX
Copy link
Contributor Author

KuthorX commented Sep 4, 2020

Could someone help me to review lastest commit? Any suggestion will be appreciated.

@PoojaDurgad PoojaDurgad added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2020
@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
Contributor

aduh95 commented Sep 19, 2023

Ping @nodejs/http for reviews.

@ShogunPanda
Copy link
Contributor

The changes look fine to me. There is only one test failing as you're using port 80. Can you use another port?

@KuthorX
Copy link
Contributor Author

KuthorX commented Oct 9, 2023

The changes look fine to me. There is only one test failing as you're using port 80. Can you use another port?

thanks, has changed to 8080

@KuthorX KuthorX requested a review from mcollina October 10, 2023 09:39
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2023
@nodejs-github-bot
Copy link
Collaborator

@KuthorX
Copy link
Contributor Author

KuthorX commented Oct 11, 2023

looks like some ci fail, maybe I should merge main branch? @ShogunPanda

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Dec 19, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 19, 2023
@nodejs-github-bot nodejs-github-bot merged commit c925039 into nodejs:main Dec 19, 2023
66 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in c925039

ionicmc-js pushed a commit to ionicmc-js/node that referenced this pull request Dec 19, 2023
PR-URL: nodejs#33633
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: James M Snell <jasnell@gmail.com>
@KuthorX
Copy link
Contributor Author

KuthorX commented Dec 19, 2023

After 3 years, it merged, thank you guys 😌

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
PR-URL: #33633
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #33633
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: James M Snell <jasnell@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants