proposal: output proper exit codes about zora HOT 25 CLOSED

@lorenzofox3 possible suggestion, what if we use a flag to tell it's running against node, and that way you can output the correct exit code, eliminating the need to pipe anything just for exit code in CI (less deps).

Something like this:
node test-file.js --node

The problem whether you read it from a flag or not is that at some point we would need in zora something like

if(typeof process !== 'undefined'){
   const code = failing ? 1 : 0;
   process.exit(code);
}

My experience has taught me this can be problematic for module bundlers for example: when they parse that branch they must understand this branch is only relevant for a given environment. It is even more troublesome as process is a native module and they might need to provide a shim for browsers, etc.

I am not saying that is not doable but it is quite a lot of trouble compared to adding a development dependency

That or just update the README and tell people it's not giving proper exit codes. People like @TehShrike had already adopted zora on a number of his repos, only to find out it was giving him false positive because CI sees it's always returning zero exit code.

I agree although most of the time as you run your tests locally first you should have few false positives and update your ci setup little by little as you update the code in your repos

I should definitely update the Readme though. Thanks for the advice

from zora.

@thisguychris, I think you don't fully understand:

1. zora does not handle exit codes for the following reasons:

• exit codes are platform specific (you don't have such a thing in the browsers for example)
• zora does not have any opinion on whether a failing test should result in a different exit code. Technically the test program may has run well (completed to its end) whereas you have some failing cases so should it really exit with something other than 0 ? It therefore gives the responsibility to the reporting process. I suggested tap-diff but they are plenty of others.

This behavior is expected and I will unlikely change it.

2. Your case is a bit uncommon, as you have invalid Javascript passed to node (how so?). By default in Unix if you use a pipe (|) the exit code will be the one emitted by the last process of the pipe. So in your case when nodejs fails to read your program it is silently ignored .
   The stdout passed to tap-diff process is some garbage (or empty) it can not understand which it interprets has no test has run. tap-diff has chosen to report this as a success others reporters processes might choose to do it differently.

Note: on unix you can make sure your command fails if any of the process in the pipe fails

set -o pipefail; node ./invalid.js | tap-diff; echo $?; will return 1

Note bis: If you are not happy with tap-diff reporter process you can use another one: tap-exit-code or tap-set-exit. That is the beauty of delegating the reporting to another process ! :)

from zora.

I opened a PR to change tap-exit-code to return status code 1 on bail out, so version 2 of that library will get the job done for anyone having the same issue.

from zora.

tap-exit-code@2 and tap-color@1.2 are both patched to set the correct status code on bail out.

from zora.

I had already thought about it. But exit code are platform specific and more over some test runners do not want those exit codes (for further processing), so I would say it is the job of the test runner.

However good news: with TAP you already have a lot of processor that can output the "proper" exit code.

For continuous integration platform I personally use

• tape-run if I need the tests to be run in a browser
• tape-diff for node

My "test runner" is simply node or the os itself :) and that gives you a lot of flexibility.

For example
rollup -c path/to/config | tee ./debug.js | node | tap-diff

the above command will bundle (or concat) all test files, create a debug file pipe it to a node process and ask tap-diff to process the output and set the proper exit code.

At some point, it might make sense to create a more opiniated test runner on top of zora to do that but I personally prefer the flexibility of small processes and believe zora should stay simple :)

from zora.

The problem with "node as a test runner" is that node test-file.js returns 0 even if tests fail. Even though I can see how it makes sense, this is not what I expected - it means that on several of my repos, TravisCI will always show green, even if tests have failed.

from zora.

@TehShrike yes I understand the point but it is so easy and more flexible to then use unix pipe command that I see no added value to use node specific code (keep in mind that some people use the browser as test runner)

For example your package json can be

devDependencies:{
  zora:"*",
  tap-diff:"*"
},
scripts:{
  "test":"node myTestFile | some-dev-adapted-tap-reporter",
  "test:ci": "node myTestFile | tap-diff" // or any other
}

and your travis build will be all fine (failing when it has to, passing when it has to)

from zora.

@lorenzofox3 possible suggestion, what if we use a flag to tell it's running against node, and that way you can output the correct exit code, eliminating the need to pipe anything just for exit code in CI (less deps).

Something like this:
node test-file.js --node

That or just update the README and tell people it's not giving proper exit codes. People like @TehShrike had already adopted zora on a number of his repos, only to find out it was giving him false positive because CI sees it's always returning zero exit code.

Super clean codebase by the way! @lorenzofox3

from zora.

@lorenzofox3 this one just bit me,

There was a syntax error, but the test returned zero test ran and it reported status zero and piped to tap-diff which should be wrong, this test runner basically passes on syntax error.

from zora.

It seems to me you have an uncaught error, zora should bail-out re throws the error then it depends on how the tap-reporter downstream deals with it.

That is the expected behavior for me at least.

There is a remaining issue though: the process does not exit with 1.
But this is due to node problem: when zora re throws the error it "produces" an unhandled promise rejection.
This should be fixed soon (see the deprecation warning) though

from zora.

This should probably be moved to a new issue - it looks like the root problem for @thisguychris's latest report is that a syntax error caused zora to spit out 0 passing and 0 failing tests, which the downstream tap reporter interpreted as a passing test.

The fix for this would probably be to say that if a script throws an error before zora spits out any test output, zora should spit out something like

not ok 1 - script threw an error before any tests were started

from zora.

I don't agree, according to to TAP specs, it should bail-out which I believe it does (see my screen shot).

The fix for this would probably be to say that if a script throws an error before zora spits out any test output, zora should spit out something like not ok 1 - script threw an error before any tests were started

Moreover your test point would not reference any actual test

If tap-diff does not handle bail out it is a different story...

Maybe @thisguychris you could provide a sample to reproduce so we can be sure we are talking about the the same thing :)

from zora.

Oh yup, you're right Bail out! is correct.

from zora.

@lorenzofox3 so what to do here? should I wait for your code to be released where zora supposed to be bailout?

My error is just from syntax error which I believe is the same as invoking an error yourself like you did in the screenshot?

from zora.

@thisguychris I think we're waiting for you to post an example test that reproduces the issue

from zora.

@TehShrike , @lorenzofox3

sorry about the long wait, but like I said it's similar to @lorenzofox3 showed, in this version I'm just showing that it passes even on syntax errors.

from zora.

This should probably be re-opened?

from zora.

I don't think so, the problem comes from the tap processor the stdout stream is piped to (tap-diff?).
If you try to run the provided program without any processor, well node will crash as it is invalid javascript

from zora.

You're right. @thisguychris I can't reproduce the issue you showed. When I clone the latest version of zora and try to run the test with bad syntax as in your example, I just get

joshduff @ Joshs-Mac-mini : ~/code/zora (master)
$  node bug-20.js 
/Users/joshduff/code/zora/bug-20.js:5
}
^

SyntaxError: missing ) after argument list
    at new Script (vm.js:79:7)
    at createScript (vm.js:251:10)
    at Object.runInThisContext (vm.js:303:10)
    at Module._compile (internal/modules/cjs/loader.js:656:28)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:699:10)
    at Module.load (internal/modules/cjs/loader.js:598:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:537:12)
    at Function.Module._load (internal/modules/cjs/loader.js:529:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:741:12)
    at startup (internal/bootstrap/node.js:285:19)

I don't see any message about tests passing, and node exists with a non-zero exit code.

from zora.

@lorenzofox3
Well, originally I was asking about zora providing exit code. You recommended to tap-diff. Now you are telling me there's something wrong with tap-diff?

@TehShrike
Try piping it to tap-diff. You'll see it's passing.

from zora.

@lorenzofox3

1. I am getting where you are coming from, it's hard to bake in code where it assumes two different environment. What I was suggesting was using a flag (--node), so that's basically telling it's opt-in to user. Which basically remove's the need to write code for detection. Anyway, I respect your opinion if you don't want it add it.

2. My case is not uncommon, but I'm not here to argue. Using exactly your code on your comment:
   #20 (comment)

Yields false positive in what you suggested - tap-diff. So even if you intentionally throw an error, the test still pass. 😐

from zora.

Yields false positive in what you suggested - tap-diff. So even if you intentionally throw an error, the test still pass. 😐

You are right, tap-diff does not handle bail out directive properly.

2. My case is not uncommon, but I'm not here to argue.

I am actually interested in that part (not the argument part : ) ), I would like to know how the devs use zora.
My assumptions is the following: most of the developers use IDE (or linters) which perform static analysis so it is very rare to have code with syntax error.

You said it is not that uncommon Is it because you are doing some sort of file watching so you file is in between two state ? or something else ? can you tell me more on the circumstances it happens.

Or are simply you referring to any error (not only invalid syntax) like in the comment ?

Thanks

from zora.

Or are simply you referring to any error (not only invalid syntax) like in the comment ?

I'm referring to any error like in the comment. In our case, the linting was disabled in the CI since like the assumption you have, "most developers have IDE / linting tools" anyway. Turns out not all of them 🤪 (the developer had his linting disabled by default). It bit us.

from zora.

I just ran into a test failure that fails properly locally but bails out in the CI environment.

Has anyone found a script that returns a correct status code on bail out?

from zora.

@TehShrike what is "the correct status code on bail out" ?

I am not sure to understand the question correctly.

The differences between your local environment and a CI setup can indeed lead to different output: missing database, different node version, different OS, etc.

If you have found a problem with zora, maybe you can open a new issue with more details so I can help to fix the issue.

from zora.