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: elevate common http sequences to common #32766

Closed
wants to merge 1 commit into from

Conversation

HarshithaKP
Copy link
Member

There are several instances of data download sequences in test
cases. Defined an abstraction for it in common module for easy
consumption.

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

There are several instances of data download  sequences in test
cases. Defined an abstraction for it in common module for easy
consumption.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 10, 2020
@HarshithaKP
Copy link
Member Author

There are many test cases that will benefit from this, but I changed only one test case now, so that any feedback can be incroporated.

@addaleax
Copy link
Member

@HarshithaKP Did you see #31968? That PR does something similar, and there is some relevant discussion in it. I would not land this unless the people who object to it are okay with this PR.

Also, the particular example here doesn’t follow best practices – when a stream body is consumed by concatenating strings, you should always use .setEncoding() (usually .setEncoding('utf8')) to make sure that multi-byte characters are handled correctly.

@HarshithaKP
Copy link
Member Author

Thanks @addaleax, I did not know about that PR.

  • my changes are only very thin wrappers around 'data' and 'end' callbacks
  • my changes do not functionally alter anything, including setEncoding
  • its objective is to reduce repeated code sequences
  • it is comaprable with common.skip - hiding 2 calls that come together

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

If we add this, it will need to be documented in test/common/README.md.

receive() is an insufficiently descriptive name for the function if we're going to add it to common.

@Trott
Copy link
Member

Trott commented Apr 13, 2020

If we add this, it will need to be documented in test/common/README.md.

receive() is an insufficiently descriptive name for the function if we're going to add it to common.

I don't want to seem like I'm moving the goalposts, so I'll add that I have other reasons I'm hesitant to land stuff like this. Those two things above are the ones I don't think anyone would disagree with. However, in general, I don't think we should be adding things to common unless the case for it is overwhelming. I don't think this meets that bar.

@HarshithaKP
Copy link
Member Author

Closing based on feedback that add things to common only when the case is overwhelming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants