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

Improve Mocha tests #739

Merged
merged 6 commits into from
Apr 20, 2021
Merged

Improve Mocha tests #739

merged 6 commits into from
Apr 20, 2021

Conversation

owenpearson
Copy link
Member

@owenpearson owenpearson commented Apr 7, 2021

Includes a few improvements to the Mocha testing set up.

95b5896 adds a convenience method for running a specific test.
39ab10c improves the console output when running karma/browserstack tests.
a8c8dae ensures that test app tear down happens after running any selection of tests.

f4acdda adds a prettier config - this was because when I was working on 106a117 I was getting a lot of annoying whitespace changes. I figured it would be better to take an opinionated approach so that these kinds of conflicts don't happen again in future. In a separate branch in future I hope to run prettier against all of our source code and add a CI check to make sure that all future code is compliant, but that was beyond the scope of this PR so I just used it on the spec/*.test.js files.

106a117 is a massive commit (sorry) which fundamentally changes the approach to async testing in mocha and refactors all of the test fixtures to use the mocha API directly. I couldn't find a reasonable way to split this in to multiple tests since most of the tests depend on helper methods which also needed to be changed, which in turn broke other tests, etc. Before we were using an assertion counter which seemed to occasionally fail tests due to some race condition. This commit changes the async tests to control when the test is complete by keeping tests open until done is called, and using lots of try/catch statements to try and catch errors and call done with those errors (causes the test to fail). This seems to have made the tests less flaky and means that they now usually fail with descriptive error messages (where before they would wait 60s and then fail with a timeout error).

63e0d5f is a workaround due to actions/setup-node#214

@owenpearson owenpearson mentioned this pull request Apr 7, 2021
@owenpearson owenpearson requested review from SimonWoolf and QuintinWillison and removed request for SimonWoolf April 7, 2021 09:21
Copy link
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to land this though I'm a little troubled by 63e0d5f as it seems like hammer to crack a nut and not immediately obviously related to the scope of this pull request.

Also worth noting that the package lock file version has increased meaning a lot of noise in the diff there, which could perhaps have impacted the npm ci vs npm install thing. 🤷‍♂️

I've not spent long looking at 106a117 as it is, indeed, massive! I'll trust you that the tests that were running before this change are still running.

@owenpearson owenpearson merged commit ceac0eb into main Apr 20, 2021
@owenpearson owenpearson deleted the improve-mocha-tests branch April 20, 2021 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants