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: increase coverage for stream writable #41486

Merged
merged 1 commit into from
Jan 17, 2022

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 12, 2022

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 12, 2022
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jan 12, 2022
@Trott Trott added the stream Issues and PRs related to the stream subsystem. label Jan 12, 2022
@Trott
Copy link
Member Author

Trott commented Jan 12, 2022

Because of the bug fix in #41433, this test will pass on the current master branch. However, prior that fix, the test here would result in an error:

TypeError: Cannot read properties of undefined (reading 'enumerable')
    at Array.<anonymous> (/Users/trott/io.js/test/common/index.js:606:35)
    at Array.<anonymous> (/Users/trott/io.js/test/common/index.js:401:15)
    at errorBuffer (node:internal/streams/writable:523:25)
    at afterWrite (node:internal/streams/writable:502:5)
    at afterWriteTick (node:internal/streams/writable:485:10)
    at processTicksAndRejections (node:internal/process/task_queues:82:21)

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 12, 2022
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott 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. labels Jan 14, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 14, 2022
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jan 14, 2022
@Trott Trott removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jan 14, 2022
@VoltrexKeyva VoltrexKeyva removed the needs-ci PRs that need a full CI run. label Jan 14, 2022
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 17, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 17, 2022
@nodejs-github-bot nodejs-github-bot merged commit 8de858b into nodejs:master Jan 17, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 8de858b

@Trott Trott deleted the streams-coverage branch January 18, 2022 05:04
thedull pushed a commit to thedull/node that referenced this pull request Jan 18, 2022
Refs: nodejs#41433 (comment)

PR-URL: nodejs#41486
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
BethGriggs pushed a commit that referenced this pull request Jan 25, 2022
Refs: #41433 (comment)

PR-URL: #41486
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
Refs: nodejs#41433 (comment)

PR-URL: nodejs#41486
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@danielleadams
Copy link
Contributor

danielleadams commented Feb 26, 2022

@Trott for some reason this test is failing when I try to pull into v16.x-staging even though the dependent commit landed in the last 16.x. Do you mind taking a look? Thanks!

This is the failure:

node:assert:635
      throw err;
      ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  Comparison {
+   message: 'Cannot call end after a stream was destroyed'
-   message: 'fhqwhgads'
  }
    at Array.<anonymous> (/Users/danielleadams/Code/nodejs/node/test/common/index.js:601:12)
    at Array.<anonymous> (/Users/danielleadams/Code/nodejs/node/test/common/index.js:394:15)
    at errorBuffer (node:internal/streams/writable:522:25)
    at afterWrite (node:internal/streams/writable:501:5)
    at afterWriteTick (node:internal/streams/writable:484:10)
    at processTicksAndRejections (node:internal/process/task_queues:82:21) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: Error [ERR_STREAM_DESTROYED]: Cannot call end after a stream was destroyed
      at new NodeError (node:internal/errors:371:5)
      at errorBuffer (node:internal/streams/writable:522:26)
      at afterWrite (node:internal/streams/writable:501:5)
      at afterWriteTick (node:internal/streams/writable:484:10)
      at processTicksAndRejections (node:internal/process/task_queues:82:21) {
    code: 'ERR_STREAM_DESTROYED'
  },
  expected: { message: 'fhqwhgads' },
  operator: 'throws'
}

@Trott
Copy link
Member Author

Trott commented Feb 28, 2022

@danielleadams I haven't confirmed, but I think that test (as written, anyway) depends on #39364 (efd40ea) which is semver-major, so I think we can leave it out of 16.x and earlier. That commit reverts cleanly on master, so an easy way to test would be to do that revert and see if the test fails on master. Unfortunately, my workspace is in a state right now that compilation will take an hour or two. I'm going to try anyway. (Or maybe an easy shortcut to confirm is to ask @ronag.)

@Trott
Copy link
Member Author

Trott commented Feb 28, 2022

Confirmed: Reverting efd40ea on master branch results in the test failing the same way:

gitpod /workspace/io.js (master) $ ./node test/parallel/test-stream-writable-final-throw.js 
node:assert:635
      throw err;
      ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  Comparison {
+   message: 'Cannot call end after a stream was destroyed'
-   message: 'fhqwhgads'
  }
    at Array.<anonymous> (/workspace/io.js/test/common/index.js:618:12)
    at Array.<anonymous> (/workspace/io.js/test/common/index.js:411:15)
    at errorBuffer (node:internal/streams/writable:523:25)
    at afterWrite (node:internal/streams/writable:502:5)
    at afterWriteTick (node:internal/streams/writable:485:10)
    at processTicksAndRejections (node:internal/process/task_queues:81:21) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: Error [ERR_STREAM_DESTROYED]: Cannot call end after a stream was destroyed
      at new NodeError (node:internal/errors:372:5)
      at errorBuffer (node:internal/streams/writable:523:26)
      at afterWrite (node:internal/streams/writable:502:5)
      at afterWriteTick (node:internal/streams/writable:485:10)
      at processTicksAndRejections (node:internal/process/task_queues:81:21) {
    code: 'ERR_STREAM_DESTROYED'
  },
  expected: { message: 'fhqwhgads' },
  operator: 'throws'
}

Node.js v18.0.0-pre
gitpod /workspace/io.js (master) $

Before reverting, it had passed.

So this is consistent with efd40ea being necessary for this test to pass. Since that commit will never land on 16.x, I think we can add the dont-land-on-16.x etc. labels.

Does that sound right to you, @danielleadams?

@danielleadams
Copy link
Contributor

@Trott that makes sense to me. Thank you for investigating.

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. 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