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: remove faulty test case #15110

Closed
wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Aug 31, 2017

While working on #14881 the test added in da1af3d started to fail for me. So I digged into that and realized that the output is not coming from the console.log in the catch part but from "somewhere". When applying the following patch it will print something different then expected.

diff --git a/test/message/stack_overflow_async.js b/test/message/stack_overflow_async.js
index 9aefbf9557..f83104ed44 100644
--- a/test/message/stack_overflow_async.js
+++ b/test/message/stack_overflow_async.js
@@ -11,7 +11,7 @@ async function g() {
   try {
     await f();
   } catch (e) {
-    console.log(e);
+    console.log('e');
   }
 }
(node:4559) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): RangeError: Maximum call stack size exceeded
(node:4559) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

When adding a console.log() or process.stdout.write("") before the test it will not print "e" either but

RangeError: Maximum call stack size exceeded
    at f (/home/ruben/repos/node/node/t.js:23:1)
    at f (/home/ruben/repos/node/node/t.js:22:9)
    at f (/home/ruben/repos/node/node/t.js:22:9)

So the test is not working as anticipated and I removed it so #14881 can land.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 31, 2017
@BridgeAR BridgeAR mentioned this pull request Aug 31, 2017
4 tasks
@addaleax addaleax self-assigned this Aug 31, 2017
@addaleax addaleax self-requested a review August 31, 2017 13:25
@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 5, 2017

Ping @addaleax

@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 8, 2017

I would like to land this relatively soon. If there are no objections in the next two days, I would just go ahead and land this.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Sep 11, 2017
PR-URL: nodejs#15110
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
@BridgeAR
Copy link
Member Author

Landed in f154c83

@BridgeAR BridgeAR closed this Sep 11, 2017
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
PR-URL: nodejs#15110
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
jasnell pushed a commit that referenced this pull request Sep 20, 2017
PR-URL: #15110
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
@bnoordhuis
Copy link
Member

@BridgeAR Can you use that explanation in the commit log next time, please? Rule of thumb: the log should explain what changed and why - not just what, that's usually easy enough to divine from the diff.

@BridgeAR
Copy link
Member Author

@bnoordhuis will definitely do. I agree that this would help.

@BridgeAR BridgeAR deleted the remove-faulty-test branch April 1, 2019 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants