Skip to content

Only report coverage for src and apps folders#14

Merged
rplopes merged 1 commit intomasterfrom
enhancement/ignore-migrations-folder-for-tests
Jun 14, 2019
Merged

Only report coverage for src and apps folders#14
rplopes merged 1 commit intomasterfrom
enhancement/ignore-migrations-folder-for-tests

Conversation

@ricardobcl
Copy link
Copy Markdown
Contributor

No description provided.

@ricardobcl ricardobcl added the enhancement New feature or request label Apr 29, 2019
@ricardobcl ricardobcl requested a review from rplopes April 29, 2019 18:04
@ricardobcl ricardobcl self-assigned this Apr 29, 2019
Comment thread configs/jest-preset.json Outdated
"*/**/*.js",
"!config/**/*",
"!coverage/**/*",
"!migrations/**/*",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We also have migrations under database/migrations/. We should probably include that one as well.

Alternatively, if we're seeing this list grow too much, should we switch the logic and apply a whitelist instead? Do we care for more directories other than apps and src?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not against whitelisting src and apps, seems more generic.

Shall we change it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

test, bin?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

test, bin?

You want coverage for test?
And bin usually is scripts, which I think we don't test (currently it's only searching for *.js files).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We're currently blacklisting test, so I don't think we'd want to start whitelisting it.

About bin, I'm on the fence. We can add it, as we may indeed have JavaScript files there, but I was under the impression that most of the times that directory was for CLI commands that might be tricky to test with jest and that defer all testable logic to a separate module in src anyway.

@ricardobcl ricardobcl force-pushed the enhancement/ignore-migrations-folder-for-tests branch from dbc5e38 to ae57f42 Compare April 30, 2019 14:51
@ricardobcl ricardobcl changed the title Ignore migrations folder for testing Only report coverage for src and apps folder Apr 30, 2019
@ricardobcl ricardobcl changed the title Only report coverage for src and apps folder Only report coverage for src and apps folders Apr 30, 2019
Copy link
Copy Markdown
Contributor

@rplopes rplopes left a comment

Choose a reason for hiding this comment

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

As per our discussion, LGTM for switching from blacklisting to whitelisting. This should also help in reducing the number of customisations to this setting in our repos, which is nice 👍

@rplopes rplopes merged commit d571939 into master Jun 14, 2019
@rplopes rplopes deleted the enhancement/ignore-migrations-folder-for-tests branch June 14, 2019 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants