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

QUnit should be semaphore-clean, config.semaphore < 0 should be an assertion error #314

Closed
masklinn opened this issue Sep 5, 2012 · 5 comments

Comments

@masklinn
Copy link

masklinn commented Sep 5, 2012

Currently, QUnit will more than once call start while the semaphore is already 0, entering a config.semaphore < 0 branch which resets it to 0.

The issue here is that this may (and does) potentially hide test failures especially in complex async cases, where the next case may partially make up for the current one and the like, ending up with a test failure for a completely unrelated piece of code.

I just finally fixed an issue of intermittent failure of a test case, where a number of tests were not obviously incorrect but randomly failed within the suite.

The problem turned out to be in the previous test file (the tests are split by module across multiple JS files, all run through the same html file) in which a test which should have been asynchronous (and correctly tried to call start) didn't stop the runner (via asyncTest, the async argument to test or stop). While trying to understand what failed where (the first symptoms that there was something screwy in the async running was that a test started before an other one ended), I realized that although QUnit has a flag to ensure every call to stop is matched to a call to start, it also ignores start-ing an already started runner. For the case above, had QUnit generated some sort of error I'd have realized sooner that I had mismatched stop/start calls.

Now I'm not putting the fault of qunit, it was quite clearly my own oversight, but I do believe there would be value in ensuring a coherent usage of the semaphore.

@jzaefferer
Copy link
Member

So whenever a start() call happens while the semaphore is already 0, it should throw an error?

@masklinn
Copy link
Author

masklinn commented Sep 5, 2012

I believe so, or — better — log a false assertion of some sort. Note that this doesn't currently work as qunit takes advantage of the current behavior.

@jzaefferer
Copy link
Member

I've implemented that, with a little workaround to allow QUnit.load() to call QUnit.start() anyway. Sanity-checked against jQuery UI, which has plenty async tests. No issues there.

//cc @dmethvin @scottgonzalez looks okay?

@scottgonzalez
Copy link
Contributor

This seems fine, but what happens when autostart is false? Shouldn't the semaphore always start at 1?

@jzaefferer
Copy link
Member

Thanks Scott, addressed that in 65ade40

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

No branches or pull requests

3 participants