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.

Sign up for GitHub

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

Jump to bottom

stream, test: Add test for _writableState state machine #8686

Closed
mcollina opened this issue Sep 21, 2016 · 12 comments
Closed

stream, test: Add test for _writableState state machine #8686

mcollina opened this issue Sep 21, 2016 · 12 comments
Labels
good first issue Issues that are suitable for first-time contributors. stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.

Comments

@mcollina
Copy link
Member

mcollina commented Sep 21, 2016 •
edited
Loading

  • Version: all
  • Platform: all
  • Subsystem: stream, test

Part of #8644

stream._writableState.needDrain
stream._writableState.ending
stream._writableState.ended
stream._writableState.finished

node/lib/_stream_writable.js

Lines 46 to 52 in 774146d

this.needDrain = false;
// at the start of calling end()
this.ending = false;
// when end() has been called, and returned
this.ended = false;
// when 'finish' is emitted
this.finished = false;

cc @Fishrock123 @nodejs/streams
@mcollina mcollina added stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests. good first issue Issues that are suitable for first-time contributors. labels Sep 21, 2016
This was referenced Sep 22, 2016
Closed
Closed
Closed
@corykitchens
Copy link

@mcollina @Fishrock123 I'd love to take a shot at this as a first contribution. Who would be the main point of contact in regards to questions/comments/etc?

@mcollina
Copy link
Member Author

You can target @nodejs/stream, or myself.

@italoacasas
Copy link
Contributor

@corykitchens I already have 3/4 PR open, the only available is the needDrain flag, if you want to do it let me know and I just move to another Issue.

@mcollina
Copy link
Member Author

@italoacasas unfortunately @nodejs/streams or myself was not tagged, and github do not send notification of open PR referencing an issue, sigh.

I'll get them reviewed asap.

@corykitchens
Copy link

@italoacasas Yes sounds good, I'll work on whatever needs to get done.

@mcollina
Copy link
Member Author

@italoacasas Can you please just use stream for the tests, instead of files? It makes things harder to port to readable-stream otherwise.

@italoacasas
Copy link
Contributor

italoacasas commented Sep 26, 2016 •
edited
Loading

@corykitchens it's at to you, just let me know.

@mcollina I'll make the change tonight, thanks for the feedback.

@italoacasas italoacasas mentioned this issue Sep 27, 2016
Closed
4 tasks
@mcollina
Copy link
Member Author

@italoacasas can you please add something in your tests, they all look the same for those variables, but they are not. I would really like to see a tests where those variables are different.

@italoacasas
Copy link
Contributor

italoacasas commented Sep 30, 2016 •
edited
Loading

@mcollina I think I understand what you mean, I'm going to refactor the tests. Thanks for the feedback.

@mcollina
Copy link
Member Author

mcollina commented Sep 30, 2016 •
edited
Loading

@italoacasas you are welcome! Let me know if you need any help, I will leave the PR non-merged atm waiting for another one.

mcollina pushed a commit that referenced this issue Sep 30, 2016
@italoacasas @mcollina
test: stream writable ended state
5e6bd84 
PR-URL: #8778
Ref: #8686
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this issue Oct 6, 2016
@italoacasas @jasnell
test: stream writable ended state
2ecd3b5 
PR-URL: #8778
Ref: #8686
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 6, 2016
@italoacasas
test: stream writable ended state
89f2221 
PR-URL: #8778
Ref: #8686
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 10, 2016
@italoacasas
test: stream writable ended state
81e60f4 
PR-URL: #8778
Ref: #8686
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this issue Oct 11, 2016
@italoacasas @Fishrock123
test: stream writable ended state
4fa0fc5 
PR-URL: #8778
Ref: #8686
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this issue Oct 18, 2016
@italoacasas @rvagg
test: stream writable ended state
5e5d034 
PR-URL: #8778
Ref: #8686
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 26, 2016
@italoacasas @MylesBorins
test: stream writable ended state
c543f4a 
PR-URL: #8778
Ref: #8686
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
mcollina pushed a commit that referenced this issue Oct 28, 2016
@italoacasas @mcollina
test: writable stream finished state
f12338d 
Add a test for _writableState.finished.

PR-URL: #8791
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Related: #8686
mcollina pushed a commit that referenced this issue Oct 28, 2016
@italoacasas @mcollina
test: writable stream ending state
fd16eed 
Add a test for _writableState.ending, when ending becomes true,
but the stream is not finished/ended yet.

PR-URL: #8707
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Related: #8686
mcollina pushed a commit that referenced this issue Oct 28, 2016
@italoacasas @mcollina
test: writable stream needDrain state
9ed7261 
Add a test for _writableState.needDrain.

PR-URL: #8799
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Related: #8686
@mcollina
Copy link
Member Author

Fixed in f12338d fd16eed
9ed7261

(I did a mess with the commit SHA in my own fork, it should be ok now).

@mcollina
Copy link
Member Author

Thanks @italoacasas for your help!!!

evanlucas pushed a commit that referenced this issue Nov 3, 2016
@italoacasas @evanlucas
test: writable stream finished state
d6f688a 
Add a test for _writableState.finished.

PR-URL: #8791
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Related: #8686
evanlucas pushed a commit that referenced this issue Nov 3, 2016
@italoacasas @evanlucas
test: writable stream ending state
248a320 
Add a test for _writableState.ending, when ending becomes true,
but the stream is not finished/ended yet.

PR-URL: #8707
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Related: #8686
evanlucas pushed a commit that referenced this issue Nov 3, 2016
@italoacasas @evanlucas
test: writable stream needDrain state
c9b67c6 
Add a test for _writableState.needDrain.

PR-URL: #8799
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Related: #8686
MylesBorins pushed a commit that referenced this issue Nov 17, 2016
@italoacasas
test: writable stream finished state
a2eedb1 
Add a test for _writableState.finished.

PR-URL: #8791
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Related: #8686
MylesBorins pushed a commit that referenced this issue Nov 17, 2016
@italoacasas
test: writable stream ending state
f1ea476 
Add a test for _writableState.ending, when ending becomes true,
but the stream is not finished/ended yet.

PR-URL: #8707
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Related: #8686
MylesBorins pushed a commit that referenced this issue Nov 17, 2016
@italoacasas
test: writable stream finished state
80a26c7 
Add a test for _writableState.finished.

PR-URL: #8791
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Related: #8686
MylesBorins pushed a commit that referenced this issue Nov 17, 2016
@italoacasas
test: writable stream ending state
ba4a3ed 
Add a test for _writableState.ending, when ending becomes true,
but the stream is not finished/ended yet.

PR-URL: #8707
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Related: #8686
MylesBorins pushed a commit that referenced this issue Nov 18, 2016
@italoacasas
test: writable stream needDrain state
4b50419 
Add a test for _writableState.needDrain.

PR-URL: #8799
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Related: #8686
MylesBorins pushed a commit that referenced this issue Nov 18, 2016
@italoacasas
test: writable stream needDrain state
40ef239 
Add a test for _writableState.needDrain.

PR-URL: #8799
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Related: #8686
MylesBorins pushed a commit that referenced this issue Nov 19, 2016
@italoacasas @MylesBorins
test: writable stream finished state
f64d93f 
Add a test for _writableState.finished.

PR-URL: #8791
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Related: #8686
MylesBorins pushed a commit that referenced this issue Nov 19, 2016
@italoacasas @MylesBorins
test: writable stream ending state
7fbfb73 
Add a test for _writableState.ending, when ending becomes true,
but the stream is not finished/ended yet.

PR-URL: #8707
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Related: #8686
MylesBorins pushed a commit that referenced this issue Nov 19, 2016
@italoacasas @MylesBorins
test: writable stream needDrain state
1fee6c1 
Add a test for _writableState.needDrain.

PR-URL: #8799
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Related: #8686
@snyk-bot snyk-bot mentioned this issue Jul 17, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Issues that are suitable for first-time contributors. stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mcollina @corykitchens @italoacasas