Fix coverage #100

sebinsua · 2020-09-17T18:57:23Z

Fixes coverage on the internal project we're currently working with.
Come up with a better commit message.

changeset-bot · 2020-09-17T18:57:25Z

🦋 Changeset is good to go

Latest commit: 236f059

We got this.

This PR includes changesets to release 1 package

Name	Type
modular-scripts	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

sebinsua · 2020-09-17T19:00:26Z

packages/modular-scripts/craco.config.js

+        setupFilesAfterEnv: globby.sync(
+          [`${absolutePackagesPath}/**/src/setupTests.{js,ts}`],
+          { cwd: process.cwd() },
+        ),


This is actually not coverage related.

I noticed that CRA would only load the setupTests.{js,ts} of one application without changing this.

is it correct to load all setupTests though? what if they're conflicting somehow?

Hm, I could change this. I somewhat agree with you.

However, if I change this there is only one application which can contain this setupTests.ts file. That's a bit confusing since it means that one application is more important than all of the others, and there's nothing to suggest that this is the case.

Perhaps we should totally change where these files are?

CRA is what configured them to be found in {app}/src/setupTests.{ts,js} and {app}/src/setupProxy.js.

yeah this one is tricky. it's almost as if setupTests should be conditionally included based on the path of the test.. not sure if possible since setupTests is like a global thing.

Yeah, this deserves an issue I think. It's likely that applications will each want their own setupTests.

Following which, it's likely each app should probably run it's own jest command.

Another thought is to have a top-level (default) setupTests, but also allow apps to provide a stripped down version of Jest config with allowed keys that allow you to either replace setupTests or return an array of [originalSetupTests, customSetupTests].

I don't think we should allow for changing the jest config. If the complication is that it'll be hard for a dev to write a command that runs all apps' jest command at once, we could do that with modular test for them. But either way, having an app define src/setupTests.js stays.

Yammered on here: #103

NMinhNguyen · 2020-09-17T19:00:20Z

packages/modular-scripts/craco.config.js

+        rootDir: absolutePackagesPath,
+        roots: ['<rootDir>'],
+        testMatch: [
+          '**/src/**/__tests__/**/*.{js,ts,tsx}',


what's the leading ../ for?

nvm, it seems like this is now **/src, although maybe it should be *? packages/*/src instead of allowing arbitrary nesting

LOL, yes, that was a mistake.

I tested the code internally but then did the open-source contribution via my own machine and did that by accident...

maybe it should be *?

Yes, I'll go and check to see if this works.

It turns out it couldn't be just a * because testMatches seems to not be in relation to <rootDir> (it's just a basic glob) while on the other hand collectCoverageFrom is by default.

However, I was able to prefix it with <rootDir>/* which I think is even more clear. And, even though collectCoverageFrom is by default prefixed by <rootDir> it ignores this if use it as a prefix so I was able to make them consistent with each other.

NMinhNguyen · 2020-09-17T19:01:01Z

packages/modular-scripts/craco.config.js

+        collectCoverageFrom: ['**/src/**/*.{js,ts,tsx}', '!**/*.d.ts'],
+        setupFilesAfterEnv: globby.sync(
+          [`${absolutePackagesPath}/**/src/setupTests.{js,ts}`],
+          { cwd: process.cwd() },


is this not the default?

It errored without passing it in. This also seemed weird to me...

https://github.com/mrmlnc/fast-glob#cwd i believe globby uses fast-glob under the hood. weird.

I seem to have ran into this, which is obviously fixed, however I can see that installing CRA also installs old versions of globby so that is probably why.

Actually, since I worked within the node_modules/ directory without installing globby myself it's possible that I picked up one of the old versions, and that since within this PR I'm explictly installing a new version, I'd not run into the problem once this is published.

(I will keep it 'just in case'.)

sebinsua · 2020-09-17T19:03:43Z

packages/modular-scripts/craco.config.js

+          '**/src/**/*.{spec,test}.{js,ts,tsx}',
+        ],
+        coverageDirectory: path.resolve(modularRoot, 'coverage'),
+        collectCoverageFrom: ['**/src/**/*.{js,ts,tsx}', '!**/*.d.ts'],


With CRA it would assume that <rootDir> would contain the src directory, but in this case, there are multiple src/ directories within each package so we prefix with <rootDir>/*.

sebinsua · 2020-09-17T19:05:33Z

packages/modular-scripts/craco.config.js

+          '**/src/**/__tests__/**/*.{js,ts,tsx}',
+          '**/src/**/*.{spec,test}.{js,ts,tsx}',
+        ],
+        coverageDirectory: path.resolve(modularRoot, 'coverage'),


I might as well set this here, rather than needing to via the CLI.

FYI, my assumption is that there is a shared coverage report for the whole application. I don't think people would want to have to run jest for each application separately -- but we can change this at a later date if we need to...

sebinsua · 2020-09-17T19:08:01Z

packages/modular-scripts/craco.config.js

+      return {
+        ...jestConfig,
+        rootDir: absolutePackagesPath,
+        roots: ['<rootDir>'],


This is actually the default for this property. CRA customizes it by postfixing src/ to it, but since <rootDir> would be the packages/ directory that would be wrong.

My other option was to get multiple roots by detecting every directory in the workspace packages/ directory but that seems overly complicated.

sebinsua · 2020-09-17T19:09:41Z

packages/modular-scripts/craco.config.js

+    configure(jestConfig) {
+      return {
+        ...jestConfig,
+        rootDir: absolutePackagesPath,


Tests are run in relation to the absolute packages/ directory instead of the modular root.

This makes sense to me since all of the application code would belong to this directory, and otherwise I'd need to prefix lots of these other properties with packages/.

NMinhNguyen · 2020-09-17T19:56:02Z

Come up with a better commit message.

you can just come up with a detailed changeset instead.

sebinsua · 2020-09-17T20:14:37Z

/cc @kknoerz

.changeset/yellow-apples-wink.md

sebinsua force-pushed the fix-coverage branch from 8a009a4 to cf51cbd Compare September 17, 2020 18:58

sebinsua commented Sep 17, 2020

View reviewed changes

NMinhNguyen reviewed Sep 17, 2020

View reviewed changes

sebinsua commented Sep 17, 2020

View reviewed changes

sebinsua force-pushed the fix-coverage branch 2 times, most recently from b555d16 to 61eb513 Compare September 17, 2020 19:51

NMinhNguyen approved these changes Sep 17, 2020

View reviewed changes

Fix test coverage so it is produced for all apps, packages and widgets

236f059

sebinsua force-pushed the fix-coverage branch from 61eb513 to 236f059 Compare September 17, 2020 20:10

sebinsua merged commit 50537c1 into master Sep 17, 2020

sebinsua deleted the fix-coverage branch September 17, 2020 20:16

github-actions bot mentioned this pull request Sep 17, 2020

Version Packages #101

Merged

NMinhNguyen reviewed Sep 17, 2020

View reviewed changes

.changeset/yellow-apples-wink.md Show resolved Hide resolved

This was referenced Sep 17, 2020

audit modular test’s behaviour #93

Closed

Should test configuration setupTests.{ts,js} be per-application? Should tests be run globally against the project or are they run against specific applications? #103

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix coverage #100

Fix coverage #100

sebinsua commented Sep 17, 2020 •

edited

Loading

changeset-bot bot commented Sep 17, 2020 •

edited

Loading

sebinsua Sep 17, 2020 •

edited

Loading

NMinhNguyen Sep 17, 2020

sebinsua Sep 17, 2020 •

edited

Loading

sebinsua Sep 17, 2020

NMinhNguyen Sep 17, 2020

threepointone Sep 18, 2020

threepointone Sep 18, 2020

NMinhNguyen Sep 18, 2020

threepointone Sep 18, 2020

sebinsua Sep 18, 2020

NMinhNguyen Sep 17, 2020

NMinhNguyen Sep 17, 2020

sebinsua Sep 17, 2020

sebinsua Sep 17, 2020

sebinsua Sep 17, 2020 •

edited

Loading

NMinhNguyen Sep 17, 2020

sebinsua Sep 17, 2020

NMinhNguyen Sep 17, 2020

sebinsua Sep 17, 2020 •

edited

Loading

sebinsua Sep 17, 2020 •

edited

Loading

sebinsua Sep 17, 2020 •

edited

Loading

sebinsua Sep 17, 2020 •

edited

Loading

sebinsua Sep 17, 2020 •

edited

Loading

NMinhNguyen commented Sep 17, 2020

sebinsua commented Sep 17, 2020

Fix coverage #100

Fix coverage #100

Conversation

sebinsua commented Sep 17, 2020 • edited Loading

changeset-bot bot commented Sep 17, 2020 • edited Loading

🦋 Changeset is good to go

sebinsua Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebinsua Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebinsua Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebinsua Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

sebinsua Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

sebinsua Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

sebinsua Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

sebinsua Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

NMinhNguyen commented Sep 17, 2020

sebinsua commented Sep 17, 2020

sebinsua commented Sep 17, 2020 •

edited

Loading

changeset-bot bot commented Sep 17, 2020 •

edited

Loading

sebinsua Sep 17, 2020 •

edited

Loading

sebinsua Sep 17, 2020 •

edited

Loading

sebinsua Sep 17, 2020 •

edited

Loading

sebinsua Sep 17, 2020 •

edited

Loading

sebinsua Sep 17, 2020 •

edited

Loading

sebinsua Sep 17, 2020 •

edited

Loading

sebinsua Sep 17, 2020 •

edited

Loading

sebinsua Sep 17, 2020 •

edited

Loading