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

The tests loop forever if I add a test for the --nodejs flag #1082

Open
anko opened this issue Mar 11, 2019 · 3 comments
Open

The tests loop forever if I add a test for the --nodejs flag #1082

anko opened this issue Mar 11, 2019 · 3 comments
Labels

Comments

@anko
Copy link

anko commented Mar 11, 2019

Trying to start on #880 (comment) by adding a failing test like a good developer, but adding this test—

diff --git a/test/cli.ls b/test/cli.ls
index 1013c951..0751e35f 100644
--- a/test/cli.ls
+++ b/test/cli.ls
@@ -94,3 +94,6 @@ command-eq '--no-header -bce "a!"' ['a();']
 
 # json+ast
 command-eq '-aje 1' [ JSON.stringify(LiveScript.ast '1'; null 2) ]
+
+# nodejs
+command-eq '--nodejs -v test/data/data.ls', [process.version]

—makes the tests loop forever:

looping forever terminal output

What's happening?

I tried these things to troubleshoot:

  • Sanity reset: git reset --hard ; npm i ; npm t, then made the same change, but nope, still loops forever.
  • Duplicated the last command-eq line in cli.ls. That works fine, so something about the line I'm adding specifically is making it choke.
  • Changed command.ls to spawnSync the node process instead of spawn. No difference, still loops forever.

Any ideas?


Versions just in case:

$ node -v
v11.11.0
$ lsc -v
LiveScript version 1.6.0
$ git rev-parse HEAD master
bc1c188f01298567bc689c979147829c6ac57213
bc1c188f01298567bc689c979147829c6ac57213
@rhendric
Copy link
Collaborator

rhendric commented Mar 11, 2019

It's not a loop, it's a recursive fork. Using the --nodejs option means that the test process spawns another node process with the same arguments... which means that the child process does what the parent was doing, namely, running all the tests, including the one which caused the fork...

@anko
Copy link
Author

anko commented Mar 12, 2019

Oh, I see!

There goes my plan though. 🙁 To make this test work as-is, the forked node process would need to call the provided log and die functions, for commandEq to capture and check. I could do that by piping its stdio through something that calls log/die with converted buffers from its stdin/stdout, but that would destroy performance in non-test use.

I'd be more inclined to refactor test/cli.ls into full end-to-end tests, running a child process lsc for each and checking captured stdio.

@rhendric
Copy link
Collaborator

It might be better to create a new file in test for end-to-end tests, rather than refactor all of test/cli.ls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants