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: stream readable readableListening internal state #9864

Closed
wants to merge 1 commit into from
Closed

test: stream readable readableListening internal state #9864

wants to merge 1 commit into from

Conversation

italoacasas
Copy link
Contributor

@italoacasas italoacasas commented Dec 1, 2016

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

stream

Description of change

Adding test for the readableListening state in stream.Readable

Issue related: #8683

cc: @mcollina

@italoacasas italoacasas added stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests. labels Dec 1, 2016
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
* `readableListening`
*/

const commun = require('../common.js');
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: common

const stream = require('stream');

/**
* Fiest test escenary, were we are going to call the
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this is supposed to say. Also, there is an extra space before the.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

me neither...

/**
* Fiest test escenary, were we are going to call the
* `readable` event and test that `readableListening` state
* change succefully change to `true`
Copy link
Contributor

Choose a reason for hiding this comment

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

successfully?

});

// readableListening state should start in `false`.
assert.deepEqual(r._readableState.readableListening, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for deepEqual(). strictEqual() is good for comparing primitives.

}));

// This should trigger some events in our
// first test ecenary
Copy link
Contributor

Choose a reason for hiding this comment

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

This can fit on the previous line I think.

* value
*/

const r2 = new stream.Readable({
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having r, r2, etc., you can place the isolated test cases inside a block scope and reuse the same variable names.

*/

const r2 = new stream.Readable({
read: (asd) => {}
Copy link
Contributor

Choose a reason for hiding this comment

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

asd?

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

The comments in general need some grammar and spell checking.

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.

Some nits on the comments.

/**
* Testing Readable Stream internal state
* `readableListening`
*/
Copy link
Member

Choose a reason for hiding this comment

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

can you remove this?

}));

// This should trigger some events in our
// second test case
Copy link
Member

Choose a reason for hiding this comment

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

can you remove this comment?

* First test case, were we are going to call the
* `readable` event and test that `readableListening` state
* change successfully change to `true`
*/
Copy link
Member

Choose a reason for hiding this comment

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

can you condense this into a one-liner?

* call the `readable` event and we expect
* that readableListening maintain his `false`
* value
*/
Copy link
Member

Choose a reason for hiding this comment

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

can you condense this into a oneliner?

@italoacasas
Copy link
Contributor Author

nits resolved

@mcollina
Copy link
Member

mcollina commented Dec 1, 2016

@mcollina
Copy link
Member

mcollina commented Dec 1, 2016

The linter seems failing.

@italoacasas
Copy link
Contributor Author

@mcollina I cannot find in the CI where is failing, can you see it ?

@mcollina
Copy link
Member

mcollina commented Dec 1, 2016

Is make lint passing?

@italoacasas
Copy link
Contributor Author

I re-run the CI, is on queue.

@italoacasas
Copy link
Contributor Author

Really funny this error Mandatory module "common" must be loaded

If i call const common = require('../common.js) I get the error

I need to call common without the .js

@italoacasas
Copy link
Contributor Author

The CI failure looks not related with the PR, can someone confirm ?

@Trott
Copy link
Member

Trott commented Dec 9, 2016

CI failure is unrelated.

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

@italoacasas
Copy link
Contributor Author

Landed 8dbf1af

Fishrock123 pushed a commit that referenced this pull request Dec 13, 2016
PR-URL: #9864
Refs: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@italoacasas italoacasas mentioned this pull request Dec 15, 2016
MylesBorins pushed a commit that referenced this pull request Jan 16, 2017
PR-URL: #9864
Refs: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 16, 2017
PR-URL: #9864
Refs: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
PR-URL: #9864
Refs: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
PR-URL: #9864
Refs: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This was referenced Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
PR-URL: #9864
Refs: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
PR-URL: #9864
Refs: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants