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: TextDecoder custom inspection #24166

Closed

Conversation

robin-drexler
Copy link
Contributor

These tests ensure hidden fields are shown when inspecting with
showHidden and that passing negative depth prints simplified value.

Co-authored-by: ZauberNerd [email protected]

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 6, 2018
@addaleax addaleax added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2018
@gireeshpunathil
Copy link
Member

@gireeshpunathil
Copy link
Member

@robin-drexler - CI fails for this PR, which I guess is related, please have a look:

04:21:38 not ok 1993 parallel/test-whatwg-encoding-textdecoder
04:21:38   ---
04:21:38   duration_ms: 0.165
04:21:38   severity: fail
04:21:38   exitcode: 1
04:21:38   stack: |-
04:21:38     internal/encoding.js:435
04:21:38               throw new ERR_NO_ICU('"fatal" option');
04:21:38               ^
04:21:38     
04:21:38     TypeError [ERR_NO_ICU]: "fatal" option is not supported on Node.js compiled without ICU
04:21:38         at new TextDecoder (internal/encoding.js:435:17)
04:21:38         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_withoutintl_x64/test/parallel/test-whatwg-encoding-textdecoder.js:95:15)
04:21:38         at Module._compile (internal/modules/cjs/loader.js:722:30)
04:21:38         at Object.Module._extensions..js (internal/modules/cjs/loader.js:733:10)
04:21:38         at Module.load (internal/modules/cjs/loader.js:620:32)
04:21:38         at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
04:21:38         at Function.Module._load (internal/modules/cjs/loader.js:552:3)
04:21:38         at Function.Module.runMain (internal/modules/cjs/loader.js:775:12)
04:21:38         at startup (internal/bootstrap/node.js:300:19)
04:21:38         at bootstrapNodeJSCore (internal/bootstrap/node.js:833:3)
04:21:38   ...

@ZauberNerd ZauberNerd force-pushed the test-textencoder-inspect branch 2 times, most recently from bd53ab9 to 5c2cf21 Compare November 13, 2018 00:35
@ZauberNerd
Copy link
Contributor

ZauberNerd commented Nov 13, 2018

@gireeshpunathil yes, these failures were because of our changes - we used the option fatal: true to trigger the code path that we wanted to test, but did not realize that this option is not available on certain builds.
I have changed the option to ignoreBOM: true (and updated the tests accordingly) and rebased against current master.

Edit: Also, since I rebased and made actual changes to the commit and code, we should invalidate all approvals on this PR?

@gireeshpunathil
Copy link
Member

@robin-drexler - thanks. the sanity test failed because:
the first letter of the first word after the subsystem name test: TextDecoder custom inspection should have been small letter, according to the rule!

we can fix it while landing, so no worries.

Also, since I rebased and made actual changes to the commit and code, we should invalidate all approvals on this PR?

@jasnell @addaleax @watilde @cjihrig @BridgeAR PTAL.

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

@gireeshpunathil
Copy link
Member

@robin-drexler - linter failed with:
First word after subsystem(s) in title should be lowercase. title-format

so basically the T in your commit message: test: TextDecoder custom inspection is the culprit. Will you amend the commit message? thanks.

@gireeshpunathil
Copy link
Member

CI failure seems to be unrelated, however re-running to be doubles-sure.
Resume CI: https://ci.nodejs.org/job/node-test-commit/23180/

@Trott
Copy link
Member

Trott commented Nov 13, 2018

Failure on shared-lib without-intl CI seems related:

https://ci.nodejs.org/job/node-test-commit-linux-containered/8570/nodes=ubuntu1604_sharedlibs_withoutintl_x64/console

04:51:17 not ok 1999 parallel/test-whatwg-encoding-textdecoder
04:51:17   ---
04:51:17   duration_ms: 0.142
04:51:17   severity: fail
04:51:17   exitcode: 1
04:51:17   stack: |-
04:51:17     assert.js:86
04:51:17       throw new AssertionError(obj);
04:51:17       ^
04:51:17     
04:51:17     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
04:51:17     + actual - expected
04:51:17     
04:51:17     + "TextDecoder {\n  encoding: 'utf-8',\n  fatal: false,\n  ignoreBOM: true,\n  [Symbol(flags)]: 4,\n  [Symbol(handle)]:\n   StringDecoder {\n     encoding: 'utf8',\n     [Symbol(kNativeDecoder)]: <Buffer 00 00 00 00 00 00 01> } }"
04:51:17     - "TextDecoder {\n  encoding: 'utf-8',\n  fatal: false,\n  ignoreBOM: true,\n  [Symbol(flags)]: 4,\n  [Symbol(handle)]: {} }"
04:51:17         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_withoutintl_x64/test/parallel/test-whatwg-encoding-textdecoder.js:104:10)
04:51:17         at Module._compile (internal/modules/cjs/loader.js:722:30)
04:51:17         at Object.Module._extensions..js (internal/modules/cjs/loader.js:733:10)
04:51:17         at Module.load (internal/modules/cjs/loader.js:620:32)
04:51:17         at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
04:51:17         at Function.Module._load (internal/modules/cjs/loader.js:552:3)
04:51:17         at Function.Module.runMain (internal/modules/cjs/loader.js:775:12)
04:51:17         at startup (internal/bootstrap/node.js:300:19)
04:51:17         at bootstrapNodeJSCore (internal/bootstrap/node.js:833:3)
04:51:17   ...

Trott
Trott previously requested changes Nov 13, 2018
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test needs to be fixed...

@Trott
Copy link
Member

Trott commented Nov 14, 2018

I'm fixing the test and will try to leave the co-authorship in there, but if our tooling chokes and we can only have one author, is there a clear primary author here?

@robin-drexler
Copy link
Contributor Author

That'd be @ZauberNerd as I got the sole git attribution for other PR we've paired. @Trott thanks for offering to do the fix, I just wonder if maybe @ZauberNerd would like to do it himself, though. :)

@Trott
Copy link
Member

Trott commented Nov 14, 2018

I just wonder if maybe @ZauberNerd would like to do it himself, though. :)

Cool. I'll hold off, especially because I'm not actually sure if the the test is failing because stuff is broken or if the different result should be expected if there is no Intl compiled into the node executable.

(If it helps, please note that common.hasIntl is a thing in our tests!)

@Trott
Copy link
Member

Trott commented Nov 20, 2018

@robin-drexler @ZauberNerd Still being worked on? Or should someone else pick this up?

These tests ensure hidden fields are shown when inspecting with
`showHidden` and that passing negative `depth` prints simplified value.

Co-authored-by: Robin Drexler <[email protected]>
@ZauberNerd
Copy link
Contributor

ZauberNerd commented Nov 20, 2018

@Trott I rebased against master, amended the commit message to adhere to the linter rules and added an if/else branch for common.hasIntl to test for both variants.
Let me know if this is fine for you or if the tests should be written differently.
Another approach I thought about would be to just test for the substring that I'm actually interested in.

Edit: The Jenkins jobs have not re-run after this force-push. Do they need to be triggered manually?

@Trott
Copy link
Member

Trott commented Nov 20, 2018

The Jenkins jobs have not re-run after this force-push. Do they need to be triggered manually?

Yep!

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

@Trott Trott dismissed their stale review November 21, 2018 05:09

test fixed

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 21, 2018
These tests ensure hidden fields are shown when inspecting with
`showHidden` and that passing negative `depth` prints simplified value.

Co-authored-by: Robin Drexler <[email protected]>
PR-URL: nodejs#24166
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Nov 21, 2018

Landed in 57869bf. 🎉 Thanks for the contribution!

@Trott Trott closed this Nov 21, 2018
targos pushed a commit that referenced this pull request Nov 21, 2018
These tests ensure hidden fields are shown when inspecting with
`showHidden` and that passing negative `depth` prints simplified value.

Co-authored-by: Robin Drexler <[email protected]>
PR-URL: #24166
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
These tests ensure hidden fields are shown when inspecting with
`showHidden` and that passing negative `depth` prints simplified value.

Co-authored-by: Robin Drexler <[email protected]>
PR-URL: #24166
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
codebytere pushed a commit that referenced this pull request Jan 13, 2019
These tests ensure hidden fields are shown when inspecting with
`showHidden` and that passing negative `depth` prints simplified value.

Co-authored-by: Robin Drexler <[email protected]>
PR-URL: #24166
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: James M Snell <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
These tests ensure hidden fields are shown when inspecting with
`showHidden` and that passing negative `depth` prints simplified value.

Co-authored-by: Robin Drexler <[email protected]>
PR-URL: nodejs#24166
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Jan 15, 2019
codebytere pushed a commit that referenced this pull request Jan 29, 2019
These tests ensure hidden fields are shown when inspecting with
`showHidden` and that passing negative `depth` prints simplified value.

Co-authored-by: Robin Drexler <[email protected]>
PR-URL: #24166
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@ZauberNerd ZauberNerd deleted the test-textencoder-inspect branch December 13, 2020 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants