Fix delay option in browser

[jRiest]

This PR addresses #1799. It feels a bit weird to special-case the delay option like this, but it looks like that's the only option that's read from mocha.options prior to mocha.run() being called.

Totally open to better suggestions though.

[danielstjules]

maybe just if ('delay' in opts) ?

[danielstjules]

Thanks for the PR, and appreciate the specs! :) While it might be odd to have a special case, it's pretty understandable that some order could be implied in these invocations. That for loop just seems like a convenient and terse shortcut.

[danielstjules]

Maybe this spec could be updated? Rather than the two assert(true), we could have another assertion on the time, ensuring that some number less than delay has elapsed since (start + delay)

[jRiest]

Cool. Made those changes. Do the browser tests get run as part of npm test? It doesn't seem like it, but I might be missing something. I tried just opening the delay.html file in a browser though and the tests pass.

[danielstjules]

Not right now. I've got a local branch I started that will run them with phantomjs, so what you have is perfect!

[danielstjules]

@jRiest is it possible that the test file broke after the change? I'm getting

passes: 0 failures: 0 duration: 0s

[jRiest]

Hmm, that's strange. It seems to still be working for me when I just open up delay.html in my browser. I did have to run make clean && make all in order to re-build the mocha.js in the root of the repo, since it looks like all of the web tests use that file.

I assumed I wasn't supposed to commit updates to the built file, is that correct?

[jRiest]

Although I guess your comment raises a concern that if the delay functionality did break, the tests may just never be run and we may never know it broke.

I suppose if I move the "should have waited..." test outside of the setTimeout that would catch that error. I'll update the PR.

[danielstjules]

I assumed I wasn't supposed to commit updates to the built file, is that correct?

That's right. My mistake - I must have checked out HEAD after rebuilding mocha.js without paying attention.

I suppose if I move the "should have waited..." test outside of the setTimeout that would catch that error. I'll update the PR.

That would be perfect :) It would be great if the test failed when the delay didn't work as expected. That's what I was trying to test.

[natlibfi-arlehiko]

@jRiest what's the status of updating this PR? We also need this fix. I can look into it too, if needed.

[jRiest]

Sorry for dropping this. We ended up not needing it so it slipped my mind. Just pushed a change that should make the tests fail without this fix.

It's a little strange since the original issue was that the run function to start the tests wasn't defined so the tests didn't run at all if the delay option was enabled. In order to make sure the tests ran and failed appropriately without this fix, I just manually invoke mocha.suite.run() if the run function isn't defined. Totally open to suggestions if there's a better option though.

[natlibfi-arlehiko]

@danielstjules, is this mergeable now?

[hollomancer]

This is a valid issue. @natlibfi-arlehiko, @jRiest - closing this for now due to age, added pr-please to #1799 so this can be updated.

[jRiest]

@hollomancer I think this PR has addressed all of the comments. Is there something else that needs to be done before it can be merged?

[boneskull]

@jRiest Thanks. It wasn't your fault this was closed; sorry about that.

I have some comments if you are willing to work on this further:

• Mocha.prototype.delay() accepts no parameters; if it is called, the option is enabled, so,
• calling mocha.setup({delay: false}) here would actually enable the delay
• test/browser/** is not run by CI--it's unclear if we can even do that--but tests in this directory are of marginal value as they stand.

A good test for this would actually run Mocha (via karma-mocha) with a delay option, which would then attempt to execute a test. The test run should fail if a test was not executed. You'd want to add probably a new target in the Makefile which would run Karma with an environment variable present; then karma.conf.js would need to pick this up, set the appropriate option(s) and execute (only) the test file.

So I think, perhaps, if you're unable or unwilling to embark on that, then modifying the code in browser-entry.js to assert truthiness of the delay option would be good enough to merge, as this is a low-risk change.

[stale]

I am a bot that watches issues for inactivity.
This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue.
Thanks for contributing to Mocha!

[ScottFreeCode]

test/browser/** is not run by CI--it's unclear if we can even do that--but tests in this directory are of marginal value as they stand.

Tests in test/browser-unit/ should be run once #2900 is merged, but in the case of using specific mocha.setup options I'm not sure that will cut it -- I'm pretty sure we would need to come up with a way to run browser integration tests (i.e. do the mocha.setup and mocha.run type stuff, run each file in a separate instance of Karma if using Karma for them)...

[stale]

I am a bot that watches issues for inactivity.
This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue.
Thanks for contributing to Mocha!