-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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: add regex to assert in test_cyclic_link_protection #11622
Conversation
@Trott Any idea how to fix the failing tests ? |
@clarenced It looks like you've uncovered a small bug in the test! Aggravating as that may be to you, I say: Well done! Let's fix it! The problem is that line 209 on your test is asserting on Windows. Here's line 209: assert.strictEqual(err.path, entry); If we look at the error output, we see that c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vcbt2015\label\win10\test\tmp.0\cycles\realpath-3a ...while c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vcbt2015\label\win10\test\tmp.0/cycles/realpath-3a If you scroll all the way to the right on the This is a bug in the test but we only found it because you made the test more strict! To fix this, you need to take things like line 194: const entry = common.tmpDir + '/cycles/realpath-3a'; ...and fix them up to use const entry = path.join(common.tmpDir, '/cycles/realpath-3a'); ( Basically, anywhere that strings with Sorry about this change being a bit more work than I realized when I suggested it! Still, I think this is totally do-able. Thanks for sticking with it! |
Hi @Trott, I am trying to push my modifications on my branch, but I have an error
Should I do a |
@clarenced I guess you In any case, you should be able to push using |
Hi @Trott I have the changes as suggested, is there anything else to do ? |
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! Thanks for doing this!
Landed in 7a4adb5! 🎉 Thanks for doing this! |
PR-URL: #11622 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#11622 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#11622 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
This uses api's that don't exist in v6.x Please manually backport or add |
PR-URL: #11622 Backport-PR-URL: #13785 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: #11622 Backport-PR-URL: #13785 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: #11622 Backport-PR-URL: #13785 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs/node#11622 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test