Skip to content

test: add test cases for paramEncoding 'explicit' - #27900

Closed
oksemonenko wants to merge 1 commit into
nodejs:masterfrom
oksemonenko:cnl-2
Closed

test: add test cases for paramEncoding 'explicit'#27900
oksemonenko wants to merge 1 commit into
nodejs:masterfrom
oksemonenko:cnl-2

Conversation

@oksemonenko

@oksemonenkooksemonenko commented May 26, 2019

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

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label May 26, 2019
ryzokuken
ryzokuken reviewed May 26, 2019
View reviewed changes

@ryzokukenryzokuken left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this!

/cc @tniessen @nodejs/crypto

@ryzokukenryzokuken added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label May 26, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@tniessen

tniessen commented May 27, 2019

Copy link
Copy Markdown
Member

Thanks for the ping @ryzokuken. Key pair generation is slow, I am not sure if this is worth adding three more test cases. Apart from that, LGTM.

@ChALkeR

ChALkeR commented May 27, 2019

Copy link
Copy Markdown
Member

@tniessen There were uncovered lines and this PR should increase actual coverage.
So this should be worth it =).

@tniessen

tniessen commented May 27, 2019

Copy link
Copy Markdown
Member

@ChALkeR Yes, we should definitely add one or more tests to cover those lines. I was just questioning the number of tests :) But you are right, it does not really matter if it's one or three.

@panva

panva commented May 27, 2019

Copy link
Copy Markdown
Member

@ChALkeR Yes, we should definitely add one or more tests to cover those lines. I was just questioning the number of tests :) But you are right, it does not really matter if it's one or three.

It's ec keys, not taxing.

ryzokuken
ryzokuken approved these changes May 27, 2019
View reviewed changes

@ryzokukenryzokuken left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@ChALkeRChALkeR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 28, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@ryzokuken

ryzokuken commented May 28, 2019

Copy link
Copy Markdown
Contributor

Any idea what failed, everyone?

@nodejs-github-bot

nodejs-github-bot commented May 28, 2019

Copy link
Copy Markdown
Collaborator

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

@ryzokuken

ryzokuken commented May 29, 2019

Copy link
Copy Markdown
Contributor
03:26:26 /home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/out/Release/obj.target/v8_base/deps/v8/src/bootstrapper.o:(.toc1+0x218): undefined reference to `v8::internal::StatisticsExtension::kSource'
03:26:26 /home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/out/Release/obj.target/v8_base/deps/v8/src/bootstrapper.o:(.toc1+0x228): undefined reference to `vtable for v8::internal::StatisticsExtension'
03:26:26 collect2: error: ld returned 1 exit status
03:26:26 make[2]: *** [/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/out/Release/mksnapshot] Error 1

Seems to be a plinux flake, I'm seeing this multiple times everyday on unrelated PRs.

@Trott we're tracking this one, right?

@BridgeAR

BridgeAR commented May 29, 2019

Copy link
Copy Markdown
Member

@oksemonenko thanks a lot and I hope you enjoyed the event!

@oksemonenko

oksemonenko commented May 29, 2019

Copy link
Copy Markdown
ContributorAuthor

@oksemonenko thanks a lot and I hope you enjoyed the event!

@BridgeAR of course I did! Thank you very much for making my sunday!

@ryzokuken

ryzokuken commented May 29, 2019

Copy link
Copy Markdown
Contributor

@BridgeAR since the CI fail is flaky, I guess it's fine to rerun CI?

@nodejs-github-bot

nodejs-github-bot commented May 29, 2019

Copy link
Copy Markdown
Collaborator

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

@tniessen

tniessen commented May 30, 2019

Copy link
Copy Markdown
Member

CI is green and the last failure looks a lot like a flake:

08:39:59 not ok 2562 known_issues/test-vm-timeout-escape-queuemicrotask
08:39:59 ---
08:39:59 duration_ms: 0.448
08:39:59 severity: fail
08:39:59 stack: |-
08:39:59 ...

So this should be good to go.

@Trott

Trott commented May 30, 2019

Copy link
Copy Markdown
Member

Landed in 76cffdd.

@TrottTrott closed this May 30, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request May 30, 2019
@oksemonenko@Trott
test: add test cases for paramEncoding 'explicit'
76cffdd
PR-URL: nodejs#27900
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@tniessentniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 30, 2019
targos pushed a commit that referenced this pull request May 31, 2019
@oksemonenko@targos
test: add test cases for paramEncoding 'explicit'
6f9aa3f
PR-URL: #27900
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targostargos mentioned this pull request Jun 3, 2019
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes
@tniessentniessentniessen approved these changes
@BridgeARBridgeARBridgeAR approved these changes
+1 more reviewer
@ryzokukenryzokukenryzokuken approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

code-and-learnIssues related to the Code-and-Learn events and PRs submitted during the events.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@oksemonenko@nodejs-github-bot@tniessen@ChALkeR@panva@ryzokuken@BridgeAR@Trott