-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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: update assert messages to show expected and actual values #19420
Conversation
xxxxxxxxxx xxxxxxxxxx | ||
xxxxxxxxxx xxxxxxxxxx` | ||
tmpdir.refresh(); | ||
fs.writeFileSync(filename, dataExpected); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Instead of creating the file in the tmpdir, maybe it would be better to put the file permanently in test/fixtures
and read it from there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I'll make this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if CI is green/yellow
1acb95a
to
97bd124
Compare
I amended the commit because the node-test-linter test failed. |
Agreed that AIX failure seems unrelated. Let's run again to get a clean lint run and see if AIX is building OK now. |
AIX failed again but I think I fixed the build problem. One more time on AIX only: https://ci.nodejs.org/job/node-test-commit-aix/13420/ |
97bd124
to
104ff10
Compare
uses the same approach as in test-fs-readfile-pipe-large
The test originally used the contents of the test file as test data. However, if the assertion fires, the resulting message is difficult to comprehend. The test now uses a short string of regularly formatted x characters that make it easier to spot differences in the expected and actual values.
The test originally used the contents of the test file as test data. However, if the assertion fires, the resulting message is difficult to comprehend. The test now uses a short string of regularly formatted x characters that make it easier to spot differences in the expected and actual values.
The test has been updated to read a test file from the test/fixtures folder. This makes the source of the test less cluttered.
104ff10
to
cfb0cc3
Compare
Hmmm...looks like the AIX results have expired. OK, let's try this again: |
Re-running ARM: https://ci.nodejs.org/job/node-test-commit-arm/15117/ |
uses the same approach as in test-fs-readfile-pipe-large PR-URL: #19420 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
uses the same approach as in test-fs-readfile-pipe-large PR-URL: #19420 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
The test-fs-readfile-pipe unit test has been updated to display the actual and expected values when the assertion fires. The test has also been updated to use a string of regularly formatted x characters as test data so that differences between the actual and expected values are easier to spot.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes