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

[v8.x backport] util: expand test coverage for util.deprecate #16430

Closed
wants to merge 1 commit into from
Closed

[v8.x backport] util: expand test coverage for util.deprecate #16430

wants to merge 1 commit into from

Conversation

akaila
Copy link
Contributor

@akaila akaila commented Oct 24, 2017

Backporting: #16305

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
Affected core subsystem(s)

util

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. v8.x labels Oct 24, 2017
@akaila akaila closed this Oct 24, 2017
@akaila akaila reopened this Oct 24, 2017
@akaila
Copy link
Contributor Author

akaila commented Oct 24, 2017

Merge in other dependent changes as well.

@trivikr
Copy link
Member

trivikr commented Oct 24, 2017

@akaila Can you rebase instead of merge?
Details: https://github.com/nodejs/node/blob/63243bcb330408d511b3945c53719425d8b7abb8/deps/uv/CONTRIBUTING.md#rebase

@MylesBorins
Copy link
Contributor

@akaila where did you get 8ea3698 from

@akaila
Copy link
Contributor Author

akaila commented Oct 24, 2017

@trivikr I did a cherry pick as required in backporting. This is not a merge or rebase.
@MylesBorins I had to make a new commit (8ea3698) since global expect error fails the test if SSL is present. I don't know how or if this is passing in master.

@akaila
Copy link
Contributor Author

akaila commented Oct 24, 2017

FYI this is the test failure without the fix:

python tools/test.py -J --mode=release parallel/test-internal-util-assertCrypto
=== release test-internal-util-assertCrypto ===
Path: parallel/test-internal-util-assertCrypto
Mismatched function calls. Expected exactly 1, actual 0.
at Object.exports.mustCall (/Users/kaila/Developer/Node/test/common/index.js:499:10)
at Object.expectsError (/Users/kaila/Developer/Node/test/common/index.js:724:27)
at Object. (/Users/kaila/Developer/Node/test/parallel/test-internal-util-assertCrypto.js:7:30)
at Module._compile (module.js:612:30)
at Object.Module._extensions..js (module.js:623:10)
at Module.load (module.js:531:32)
at tryModuleLoad (module.js:494:12)
at Function.Module._load (module.js:486:3)
at Function.Module.runMain (module.js:653:10)

@apapirovski
Copy link
Member

You shouldn't include anything that is not directly related to the specific PR you're backporting, even if tests are failing. That can be solved in a separate PR if necessary. That said, error changes are semver-major so that fix doesn't seem applicable to v8.x anyway.

@akaila
Copy link
Contributor Author

akaila commented Oct 24, 2017

Ok I took out that commit. The only commit now is my test and its dependent code.

@MylesBorins
Copy link
Contributor

MylesBorins commented Oct 24, 2017 •
edited
Loading

@apapirovski the error change in 8ea3698 is in a test, not in core... so it wouldn't be semver major

I too am very confused as to how that is passing on master

edit: @apapirovski my bad, I'm seeing that the change to error codes is in the backport.

@apapirovski
Copy link
Member

@MylesBorins The issue I'm seeing is with a07981d811e2c22c57dbaf614a1caa94a34f30d4 being included here

@MylesBorins
Copy link
Contributor

@akaila we cannot land a07981d in v8.x as it is a semver major change. As it would appear that the backport requires the error change I'm going to go ahead and close this

My apologies for the miscommunication, thank you so much for taking the time to open this

@apapirovski
Copy link
Member

@MylesBorins I think this all got very confused. 😭 This should still be open, the issue is that it includes a07981d (incorrect) alongside 82134aa (correct)

Maybe I'm missing something though...

@apapirovski apapirovski reopened this Oct 24, 2017
@apapirovski
Copy link
Member

apapirovski commented Oct 24, 2017 •
edited
Loading

The real solution here should be to backport that test but change the error being tested for, since the error is different.

`code` argument must be a string

Without using the internal errors.

@akaila
Copy link
Contributor Author

akaila commented Oct 24, 2017

I get it now. I will revert and modify the test and update the PR. Thanks

@MylesBorins
Copy link
Contributor

hey all... common.expectsError is only for testing internal/error objects... as such I don't think this makes sense to continue with. It will be in 9.x, and the expanded coverage doesn't make sense for 8.

I will avoid closing again but I advise that we close this

@targos
Copy link
Member

targos commented Oct 24, 2017

common.expectsError should work with plain Errors too.

@apapirovski
Copy link
Member

apapirovski commented Oct 24, 2017 •
edited
Loading

@MylesBorins Is there downside to having an extra test for util.deprecate in v8.x? Even if common.expectsError doesn't work (I think it should?), we can still use assert.throws...

@MylesBorins
Copy link
Contributor

@targos gtk
@apapirovski no downside

Sorry for being a party pooper, please safely ignore me

@akaila
util: expand test coverage for util.deprecate
d38a858 
Test for invalid argument types passed to code on util.deprecate.

PR-URL: #16305
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@akaila
Copy link
Contributor Author

akaila commented Oct 24, 2017

Backported the test and corrected the error message and removed the error code. Verified it passes.

@gibfahn gibfahn force-pushed the v8.x-staging branch 2 times, most recently from 8ec47e5 to 61413c2 Compare October 30, 2017 21:27
@gibfahn gibfahn force-pushed the v8.x-staging branch 3 times, most recently from b183192 to fc8acc8 Compare October 30, 2017 21:42
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@akaila @gibfahn
util: expand test coverage for util.deprecate
908ba41 
Test for invalid argument types passed to code on util.deprecate.

PR-URL: #16305
Backport-PR-URL: #16430
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
@akaila @gibfahn
util: expand test coverage for util.deprecate
55ba1d4 
Test for invalid argument types passed to code on util.deprecate.

PR-URL: #16305
Backport-PR-URL: #16430
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@gibfahn
Copy link
Member

gibfahn commented Oct 31, 2017

@akaila thanks a lot for sticking with this!

Landed as 55ba1d4

@gibfahn gibfahn closed this Oct 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@akaila @trivikr @MylesBorins @apapirovski @targos @gibfahn @nodejs-github-bot