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_runner: stringify AssertError expected and actual #47088

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Mar 14, 2023

Fixes: #47075

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Mar 14, 2023
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.

I think this change might work, but I think it's incomplete without a context object. I also worry about how the stringify change might interact with large objects.

I'm not 100% if it would be valid YAML, but if it is, maybe actual and expected should add another level of indentation instead of just trying to stringify.

test/message/test_runner_output.js Outdated Show resolved Hide resolved
test/message/test_runner_output.js Outdated Show resolved Hide resolved
test/message/test_runner_output.out Outdated Show resolved Hide resolved
lib/internal/test_runner/reporter/tap.js Outdated Show resolved Hide resolved
@MoLow MoLow force-pushed the stringify-assert-error-props branch 3 times, most recently from 6a60ab8 to b6d1159 Compare March 21, 2023 20:17
@MoLow MoLow requested a review from cjihrig March 21, 2023 20:21
@MoLow MoLow force-pushed the stringify-assert-error-props branch from b6d1159 to b540361 Compare March 28, 2023 19:20
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.

Sorry for the delay. LGTM

lib/internal/test_runner/reporter/tap.js Outdated Show resolved Hide resolved
lib/internal/test_runner/yaml_to_js.js Show resolved Hide resolved
@MoLow MoLow force-pushed the stringify-assert-error-props branch from b540361 to 8840601 Compare April 3, 2023 16:22
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 4, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 4, 2023
@nodejs-github-bot nodejs-github-bot merged commit 102540a into nodejs:main Apr 4, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 102540a

@MoLow MoLow deleted the stringify-assert-error-props branch April 4, 2023 07:34
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
PR-URL: #47088
Fixes: #47075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
PR-URL: #47088
Fixes: #47075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023
PR-URL: #47088
Fixes: #47075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
PR-URL: #47088
Fixes: #47075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 8, 2023
PR-URL: #47088
Fixes: #47075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 13, 2023
PR-URL: #47088
Fixes: #47075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
PR-URL: #47088
Fixes: #47075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Jul 6, 2023
PR-URL: nodejs#47088
Fixes: nodejs#47075
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
needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

assert.deepEqual can generate invalid YAML output
5 participants