Normalize Mocha tests to have realistic timings

[WaleedAshraf]

Description of the Change

Add .slow() to tests which are normally taking longer time.

Benefits

More clear view of actual slow tests.

Possible Drawbacks

Should keep an eye on these test for later -- if they are getting even slower, then something's 🐟 !

Applicable issues

Fixes #2597
semver-patch

See also #2837

[jsf-clabot]

All committers have signed the CLA.

[coveralls]

Coverage increased (+0.07%) to 91.888% when pulling 2d19e82 on WaleedAshraf:issue/2597 into ffbcbf6 on mochajs:master.

[plroebuck]

Haven't reviewed the rest, but definitely revert 6b93c95. Changes to test specifications do not require any changes to "package.json".

[WaleedAshraf]

@plroebuck done.

[plroebuck]

Could you explain the process you used to come up with your slow threshold values?

[WaleedAshraf]

I ran tests for 3,4 times and added (~3 * average value of duration it took)

If a test takes 100ms

• slow(100) would be red
• slow (200) would orange/yellow
• slow (300) would be green
• slow (>300) would be no warning

e.g All of options.spec.js tests are taking ~250-300ms avg time,
so, slow(800) threshold is okay for them.

Also, if it's a semver-patch, then we need to update package.json with version bump.

[plroebuck]

Were the tests run on AppVeyor?

semver-patch annotates its ability to be merged directly to master here; the package version itself will change at next release.

[WaleedAshraf]

@plroebuck no. I ran them locally.
Is AppVeyor possible on local? Or maybe I can re-run the AppVeyor job here?

Got it 👍

[plroebuck]

@plroebuck no. I ran them locally.
Is AppVeyor possible on local? Or maybe I can re-run the AppVeyor job here?

Yeah, local timings won't be as much help. Need to run against the 80386 machines Appveyor seems to be powered by... :)

You could use timings from our existing commits. Try to get some spread between when you collect results -- certain times of day are much worse than others.

[plroebuck]

BTW, you are using the actual formula, right?

[boneskull]

this also resolves #2483... lots of dupes.

[boneskull]

thanks! just noticed one issue with the code.

I haven't had a chance to cross-reference the values and the real-world perf though

[boneskull]

typo

[plroebuck]

#2483 already closed.

[WaleedAshraf]

@plroebuck ok. I'll cross check with previous builds and will update values where needed.
And yes, I used the same formula.

@boneskull good catch. Will push updates by this weekend. 👍

[craigtaub]

@WaleedAshraf any movement on this?

[WaleedAshraf]

Hi @craigtaub, Sorry for leaving it here for long.
I'll push changes in few days.

[WaleedAshraf]

@here, I think it's not needed now.
I check some of the last builds at https://travis-ci.org/mochajs/mocha/pull_requests
and I don't see many slow tests. Only one or two at max, which is fine.

We can close this PR with #2597 and #2837

[craigtaub]

Have you checked on AppVeyor?

To quote issue.

It's important to note that our tests run much more slowly on AppVeyor. That should be the baseline for timeout values--not your MacBook Pro.

[WaleedAshraf]

@craigtaub I did, but it's not that clear because logs are not colored and it's difficult to identify slow (red) tests.
Maybe we need to fix that also.

[craigtaub]

Ah yes, they don't support colours so that won't be possible.

Think you can tell its slow if over default slow value (for integrations believe 3750ms).

[WaleedAshraf]

Hi,
Let's get done with this.

I checked the lastest build on AppVeyor and there is only one test which hits the limit of 3750ms

regressions
    √ issue-1991: Declarations do not get cleaned up unless you set them to `null` - Memory Leak (16234ms)

But this is way too over the limit. Rest of tests are under the limit.

[outsideris]

I agreed to close this issue. Any thoughts?