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

Invalid JS in test file should cause failure, not skip file #2

Open
focusaurus opened this issue Jun 9, 2016 · 6 comments
Open

Invalid JS in test file should cause failure, not skip file #2

focusaurus opened this issue Jun 9, 2016 · 6 comments

Comments

@focusaurus
Copy link

I observe if I have a mistake in the javascript in my tape test file, regular tape will raise a SyntaxError and exit nonzero causing my tests to fail. With the same situation, changing my requires to require('tape-catch'), the invalid files are silently skipped and the tests pass (just a lower-than-expected number of tests are executed). This defeats my ability to use tests as a continuous integration guard mechanism. Would you consider printing a clear error and failing the test run with a nonzero exit code in this case?

Here's a sample test case to reproduce:

const test = require('tape-catch')

test('test B', (tape) => {
  tape.ok('forgot last close parentheses')
  tape.end()
}
@michaelrhodes
Copy link
Owner

Yep, I think returning a zero exit code is definitely a bug. I'll take a look at fixing this up for you in the next few days

@michaelrhodes
Copy link
Owner

Hm, I’m actually not sure how that sample file is able to run at all. node complains about the syntax error and returns 1 for me. I assume I’m missing something obvious, but if you could think for me that would be great, haha

@focusaurus
Copy link
Author

It's not valid js so no it can't run. This should be detected and cause the
test run to fail.

On Sat, Jun 11, 2016, 1:12 PM Michael Rhodes [email protected]
wrote:

Hm, I’m actually not sure how that sample file is able to run at all. node
complains about the syntax error and returns 1 for me. I assume I’m missing
something obvious, but if you could think for me that would be great, haha


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#2 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAdcSWKBz491nzEXJUIOimJFiu-rth50ks5qKwh_gaJpZM4IyF6t
.

@michaelrhodes
Copy link
Owner

Oh yeah, I get that, but I’m struggling to work out how tape-catch could detect those issues. Because, unless my brain isn’t functioning properly (which is possible), a syntax error outside the test callback would prevent the file from being parsed, and thus require('tape-catch') would never be evaluated.

@boxfoot
Copy link

boxfoot commented Jun 16, 2016

I'm having the same issue, for me it's coming up when importing invalid modules, which means the test file itself runs but there's an import error. I'm simulating that in this example with a crazy eval statement that accomplishes the same thing:

var test = require("tape-catch");

test('first test', function (t) {
    t.assert(true, 'test 1 passes');
    t.end();
});

eval('this is a syntax error');

test('second test', function (t) {
    t.assert(true, 'test 2 passes');
    t.end();
});

Expected result: non-zero exit code
Actual result: first test executes fine, second test is simply skipped.

If you move the eval before the first test, then there are simply no tests executed.

Commenting out this line returns the expected behavior -- but is probably losing some other important functionality.

@focusaurus
Copy link
Author

I've spent the morning looking over many different error cases comparing tape and tape-catch.

I believe the specific issue of skipping tests silently for invalid files has been resolved by [email protected].

(Aside: it would have saved me some time this morning if you had posted a comment on this issue when 1.0.6 was released. I didn't actually notice until today after I had worked a bit on digging into this issue)

For a bunch of other edge cases, we can get failing TAP instead of a stack trace if we run tape -r tape-catch as our test runner command line, so my thought is either we should document that (I can make a PR for that) or add a wrapper CLI tape-catch that is equivalent.

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

No branches or pull requests

3 participants