Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Merge jsdom and node coverage results #122

Merged
merged 2 commits into from
Jan 9, 2018

Conversation

KevinGrandon
Copy link
Contributor

@KevinGrandon KevinGrandon commented Jan 9, 2018

This creates multiple coverage folders depending on what test suites are run (node, jsdom), and merges them after coverage is collected into a single coverage/ folder. There is some weirdness around the jest process exiting before coverage is done writing to disk, so we wait until expected coverage files are present before merging.

Fixes #121

env =>
new Promise(resolve => {
const interval = setInterval(() => {
const coverageExists = fs.existsSync(
Copy link

Choose a reason for hiding this comment

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

I thought exists was being deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like exists() is being deprecated, but existsSync() is not.

const fs = require('fs');

// Jest writes coverage after the process exits
// so we need to poll and wait for all coverage files.
Copy link

Choose a reason for hiding this comment

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

What if there is an error and coverage is never written?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The node process should just exit in this case, killing the CLI as well. I'll go ahead and include a timeout here though in case coverage never gets written for some reason.

@@ -1 +1 @@
coverage/
coverage*/
Copy link

Choose a reason for hiding this comment

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

We should make sure to make this change in the scaffold.

Copy link

@mlmorg mlmorg left a comment

Choose a reason for hiding this comment

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

lgtm

@KevinGrandon
Copy link
Contributor Author

!merge

@old-fusion-bot old-fusion-bot bot merged commit 16b1516 into fusionjs:master Jan 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants