issue #973 - mark suite with failing hook(s) as pending

[lightsofapollo]

No description provided.

[lightsofapollo]

@visionmedia Proof of concept using pending... This approach makes the most the sense to me given that the tests are "pending" the hooks working correctly (and where never run so we don't know their result)... I am working on a higher level set of tests to verify that this functionality will keep working.

[lightsofapollo]

@visionmedia now with |test-integration| which will not use mocha itself to verify the sanity of mocha (just for this bug at the moment)

[lightsofapollo]

Any chance of getting this reviewed?

[Whoaa512]

The code & tests you've written look great, but not sure I fully understand the use-case here. If a hook fails, I want to know about it, but I wouldn't want it to skip the rest of the suite if the hook failure was an outlier. Maybe the hook failed for the 3rd test because the API or something happened to be extra slow at that moment.

I want the meta data associated from seeing how the hook behaved at every test run, and I feel this change is counter to that. Please help me see this from your perspective.

[lightsofapollo]

@Whoaa512 so what you want is a step further in the direction this code takes us..

Here is an example:

You have tests:

a, b, c and d

Test a is a success test b fails on suiteSetup (before). What I would expect is test c and d will continue to run so I can see the results of those files (each test should be isolated by suite). In reality what happens is mocha "end"(s) after b and no further tests are run. In the past there was a reporter bug that made it appear like those tests actually ran but they do not.

Does that clear it up?

[travisjeffery]

did a quick pass and it looks ok. i'll take a closer look this weekend.

[travisjeffery]

so i just took a closer look. what i did was remove this commit lightsofapollo@cd9a706, leaving just this commit lightsofapollo@57840e8 which you say should then fail. but it passes... this is why you write and run the tests first... ;)

[lightsofapollo]

@travisjeffery that is not the behavior I have locally when I revert the changeset but that is on mocha as of a month~ ago when I submitted the patch... I will rebase and see what is going wrong.

[lightsofapollo]

@travisjeffery hmm looks like this works as expected... what did you do differently locally? Is there something else you want me to address ?

[travisjeffery]

what exactly do you mean works as expected? as in the tests are failing and you expected that?

i put up a branch with the integration test commit of yours that you said should be failing now. but they're passing when i run them locally. branch name issue-975: https://github.com/visionmedia/mocha/tree/issue-975. the hooks seem to be working correctly, no?

[lightsofapollo]

@travisjeffery
I have no idea why this works locally for you... Above I was referring to when I reverted the commit with the new functionality the tests failed:

https://travis-ci.org/visionmedia/mocha/builds/12777835

The branch you pushed also fails on travis with the error I expect: https://travis-ci.org/visionmedia/mocha/builds/12773502

---

What are you doing locally to run the tests? Maybe something is wrong with how I setup the integration target or ... ?

[travisjeffery]

ah here we go, i was dumb and forgot make test doesn't run the integration tests. it's failing now with make test-all

[lightsofapollo]

@travisjeffery great! I rebased so the tests are green again (popped off my revert commit)

[travisjeffery]

nice. one more thing: is there any reason why we can't remove test/integration/hook_fail_before_each.js and move that stuff into Makefile so it's similar to the rest of the tests?

[lightsofapollo]

@travisjeffery

I was trying to follow the conventions I saw in the Makefile make test is actually make test-unit it seemed logical to add make test-integration and add it to the make test-all target. Do you want me to add this to the test target ?

It looks like the failing test is from this merge: https://travis-ci.org/visionmedia/mocha/builds/12472035

[travisjeffery]

adding test-integration is fine.

but look at how the other test-* (e.g. test-unit, test-compiler, test-requires) execute mocha from the Makefile, where as your test-integration executes a node script and that executes mocha. So it'd be nice if test-integration was implemented as @.bin/mocha ... rather than as it is now: test/integration/hook_fail_before_each.js

[lightsofapollo]

the make target is there now its there now https://github.com/visionmedia/mocha/pull/975/files#diff-b67911656ef5d18c4ae36cb6741b7965R40

I missed the phony (which I just added)

To the second point- I wanted to isolate mocha from the runner part particularly so it could not potentially cause an error that testing mocha from the outside could catch. If it blocks this from landing I will to move it from: vanilla -> mocha to mocha -> mocha. FWIW I think it makes it harder to break by not relying on mocha to test mocha (for extreme high level cases).

[lightsofapollo]

@travisjeffery ping- If the comments about testing mocha with mocha are blocking this I will make the desired changes... I still think this is the right way to test lower level mocha features.

[travisjeffery]

@lightsofapollo blocked partly because i'm not a fan of how those tests are written (side-note: integration tests = low level?). so ya if you could write these to use mocha and be like every other test that'd be great. the other part is making sure this is the right thing since there's a lot of other similar prs/issues related to this.

[lightsofapollo]

#1043 also does what I want and is closer to what @travisjeffery wants... We can repoen this if there is something more to do here.