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: ensure stream preprocessing order #7741

Closed
wants to merge 11 commits into from
Closed

test: ensure stream preprocessing order #7741

wants to merge 11 commits into from

Conversation

vsemozhetbyt
Copy link
Contributor

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

test

Description of change

Sometimes it is necessary to preprocess some initial bit of a stream data before giving the entire stream to the main processing function. Sometimes this bit should be extracted from the stream before the main processing; sometimes it should be returned to the stream. This test checks an order of stream modes, methods and events for a possible preprocessing algorithm. Stream BOM stripping is selected as a use case.

See nodejs/help#221 as the prehistory.

Sometimes it is necessary to preprocess some initial bit
of a stream data before giving the entire stream
to the main processing function. Sometimes this bit should be extracted
from the stream before the main processing; sometimes it should be
returned to the stream. This test checks an order of stream
modes, methods and events for a possible preprocessing algorithm.
Stream BOM stripping is selected as a use case.

See nodejs/help#221 as the prehistory.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 14, 2016
@vsemozhetbyt
Copy link
Contributor Author

Sorry, this is my first test at all, and I am not sure if I have not done some stupid things.

@mscdex
Copy link
Contributor

mscdex commented Jul 15, 2016

The actual test file should be in test/parallel with the rest of the tests.

Also, I'm not sure a stream-preprocess/ subdirectory is necessary in fixtures/.

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Jul 15, 2016
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jul 15, 2016

@mscdex So should I delete stream-preprocess from the fixtures and put all three files into test/parallel? Or should I leave .txt files in the fixtures and change paths to them?

Should I also rename the .js file according to some rules?

@mscdex
Copy link
Contributor

mscdex commented Jul 15, 2016

I'd leave the text files in fixtures/ and move the test to test/parallel and prefix it like the other tests.

The changes look good now, although you will need to fix your paths.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jul 15, 2016

I've fixed them before moving, also .js is re-tested / re-eslinted.

const fs = require('fs');
const path = require('path');
const rl = require('readline');
/******************************************************************************/
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a minor nit, but in keeping with the style of most of the other tests, it's probably best to just replace this line with a blank line and remove the other two earlier asterisk lines completely.


// get the data by a non-stream way to compare with the streamed data
const modelData = fs.readFileSync(
path.join(__dirname, '..', 'fixtures', 'file-to-read-without-bom.txt'), 'utf8'
Copy link
Member

Choose a reason for hiding this comment

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

path.join(common.fixturesDir, 'file-to-read-without-bom.txt') (and below where appropriate)?

@vsemozhetbyt
Copy link
Contributor Author

@mscdex , @addaleax Thank you.


const BOM = '\uFEFF';

// get the data by a non-stream way to compare with the streamed data
Copy link
Member

Choose a reason for hiding this comment

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

nit: Comments should be capitalized and punctuated; also, maybe using is better than by?

@vsemozhetbyt
Copy link
Contributor Author

@addaleax Thank you.

@addaleax
Copy link
Member

addaleax commented Jul 25, 2016

LGTM with CI: https://ci.nodejs.org/job/node-test-commit/4243/ (Edit: is green.)

@addaleax
Copy link
Member

Landed in ea725ed, thanks!

@addaleax addaleax closed this Jul 27, 2016
addaleax pushed a commit that referenced this pull request Jul 27, 2016
Sometimes it is necessary to preprocess some initial bit
of a stream data before giving the entire stream
to the main processing function. Sometimes this bit should be extracted
from the stream before the main processing; sometimes it should be
returned to the stream. This test checks an order of stream
modes, methods and events for a possible preprocessing algorithm.
Stream BOM stripping is selected as a use case.

See nodejs/help#221 as the prehistory.

PR-URL: #7741
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@vsemozhetbyt vsemozhetbyt deleted the test-fixtures-stream-preprocess branch July 27, 2016 12:20
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
Sometimes it is necessary to preprocess some initial bit
of a stream data before giving the entire stream
to the main processing function. Sometimes this bit should be extracted
from the stream before the main processing; sometimes it should be
returned to the stream. This test checks an order of stream
modes, methods and events for a possible preprocessing algorithm.
Stream BOM stripping is selected as a use case.

See nodejs/help#221 as the prehistory.

PR-URL: #7741
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
@mcollina
Copy link
Member

This test fails on node v0.8, v0.10, v0.12 for readable-stream: https://travis-ci.org/nodejs/readable-stream/builds/153583071.

I'm not sure if this is something @nodejs/lts wants to look into for v0.12.

MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
Sometimes it is necessary to preprocess some initial bit
of a stream data before giving the entire stream
to the main processing function. Sometimes this bit should be extracted
from the stream before the main processing; sometimes it should be
returned to the stream. This test checks an order of stream
modes, methods and events for a possible preprocessing algorithm.
Stream BOM stripping is selected as a use case.

See nodejs/help#221 as the prehistory.

PR-URL: #7741
Reviewed-By: Anna Henningsen <anna@addaleax.net>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
Sometimes it is necessary to preprocess some initial bit
of a stream data before giving the entire stream
to the main processing function. Sometimes this bit should be extracted
from the stream before the main processing; sometimes it should be
returned to the stream. This test checks an order of stream
modes, methods and events for a possible preprocessing algorithm.
Stream BOM stripping is selected as a use case.

See nodejs/help#221 as the prehistory.

PR-URL: #7741
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
Sometimes it is necessary to preprocess some initial bit
of a stream data before giving the entire stream
to the main processing function. Sometimes this bit should be extracted
from the stream before the main processing; sometimes it should be
returned to the stream. This test checks an order of stream
modes, methods and events for a possible preprocessing algorithm.
Stream BOM stripping is selected as a use case.

See nodejs/help#221 as the prehistory.

PR-URL: #7741
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants