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

test_runner: stringify AssertError expected and actual #47088

Merged
merged 1 commit into from Apr 4, 2023
Merged

test_runner: stringify AssertError expected and actual #47088

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

cjihrig
cjihrig reviewed Mar 14, 2023
View reviewed changes
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
cjihrig
cjihrig approved these changes Apr 2, 2023
View reviewed changes
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

CI: https://ci.nodejs.org/job/node-test-pull-request/50870/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50878/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50883/

@github-actions github-actions bot mentioned this pull request Apr 4, 2023
Open
29 tasks
@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
55 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 102540a

@MoLow MoLow deleted the stringify-assert-error-props branch April 4, 2023 07:34
@github-actions github-actions bot mentioned this pull request Apr 5, 2023
Open
26 tasks
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
@MoLow @RafaelGSS
test_runner: stringify AssertError expected and actual
bc0062b 
PR-URL: #47088
Fixes: #47075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
@MoLow @RafaelGSS
test_runner: stringify AssertError expected and actual
243546f 
PR-URL: #47088
Fixes: #47075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023
@MoLow @RafaelGSS
test_runner: stringify AssertError expected and actual
fd1b825 
PR-URL: #47088
Fixes: #47075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
@MoLow @RafaelGSS
test_runner: stringify AssertError expected and actual
c8c7e03 
PR-URL: #47088
Fixes: #47075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 8, 2023
@MoLow @RafaelGSS
test_runner: stringify AssertError expected and actual
eb837ce 
PR-URL: #47088
Fixes: #47075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 13, 2023
@MoLow @RafaelGSS
test_runner: stringify AssertError expected and actual
1e9a500 
PR-URL: #47088
Fixes: #47075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@MoLow @danielleadams
test_runner: stringify AssertError expected and actual
a65dd0b 
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
@MoLow
test_runner: stringify AssertError expected and actual
1d453e4 
PR-URL: nodejs#47088
Fixes: nodejs#47075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@ljharb ljharb ljharb approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. test_runner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

assert.deepEqual can generate invalid YAML output
5 participants
@MoLow @nodejs-github-bot @ljharb @cjihrig @aduh95