Skip to content

Improve testRegex in jest-preset.json#18

Merged
rplopes merged 1 commit intomasterfrom
support/improve-jest-testRegex
Oct 11, 2019
Merged

Improve testRegex in jest-preset.json#18
rplopes merged 1 commit intomasterfrom
support/improve-jest-testRegex

Conversation

@waldyrious
Copy link
Copy Markdown
Contributor

@waldyrious waldyrious commented Oct 1, 2019 •
edited
Loading

  • Remove the "spec" part of the match, as none of the repos that use (or might use) uphold-scripts uses it
  • Remove the unused regex groups

@waldyrious waldyrious self-assigned this Oct 1, 2019
@waldyrious waldyrious force-pushed the support/improve-jest-testRegex branch from 2c1a3a0 to 12e24b4 Compare October 1, 2019 14:55
Americas
Americas reviewed Oct 1, 2019
View reviewed changes
Comment thread configs/jest-preset.json Outdated
"restoreMocks": true,
"testEnvironment": "node",
"testRegex": "((\\.)(test|spec))\\.js$"
"testRegex": "(\\.|_)(test|spec)\\.js$"
Copy link
Copy Markdown
Contributor

@Americas Americas Oct 1, 2019

Choose a reason for hiding this comment

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

We could also remove spec, I don't believe it is used.

Copy link
Copy Markdown
Contributor Author

@waldyrious waldyrious Oct 1, 2019

Choose a reason for hiding this comment

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

Unfortunately we do use it. It would be nice to standardize on a test naming scheme, indeed. @NelsonBrandao is this perhaps an ecosystem issue? Is *.spec.js more widespread in frontend/React projects?

Copy link
Copy Markdown
Contributor

@Americas Americas Oct 1, 2019

Choose a reason for hiding this comment

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

Meh, silly frontend 🐰

Copy link
Copy Markdown

@NelsonBrandao NelsonBrandao Oct 2, 2019

Choose a reason for hiding this comment

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

@waldyrious Frontend does not use uphold-scripts.

Copy link
Copy Markdown
Contributor Author

@waldyrious waldyrious Oct 2, 2019

Choose a reason for hiding this comment

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

Is that circumstantial, or is the plan to never use it?

Copy link
Copy Markdown

@NelsonBrandao NelsonBrandao Oct 2, 2019

Choose a reason for hiding this comment

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

Currently there are no plans to add it since some configurations are not compatible.

Copy link
Copy Markdown
Contributor Author

@waldyrious waldyrious Oct 2, 2019

Choose a reason for hiding this comment

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

Yes, but my question is, could they be made compatible? Or is there a fundamental rift we can't (or shouldn't) bridge?

Copy link
Copy Markdown

@NelsonBrandao NelsonBrandao Oct 2, 2019

Choose a reason for hiding this comment

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

I don't think conceptually it makes sense to use uphold-scripts for both frontend and backend.

This package was created to automate menial tasks between all backend repos. The frontend uses a mono-repo to solve this very same problem so there is no need for uphold-scripts.

Copy link
Copy Markdown
Contributor Author

@waldyrious waldyrious Oct 2, 2019

Choose a reason for hiding this comment

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

Great, thanks for confirming. I'll remove "spec" from the regex, then! 🎉

Copy link
Copy Markdown
Contributor Author

@waldyrious waldyrious Oct 2, 2019

Choose a reason for hiding this comment

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

Done.

@waldyrious waldyrious force-pushed the support/improve-jest-testRegex branch 2 times, most recently from 89289f5 to 1cc2084 Compare October 2, 2019 09:20
@rplopes
Copy link
Copy Markdown
Contributor

rplopes commented Oct 2, 2019

Add a new option to the first inner group, to support the test filenames currently used on the backend repo.

Is backend the only repo with such a format? If so, and since it doesn't even use jest, I'd say let's keep supporting one team convention only that all projects can converge to. Even if at some point we migrate backend to jest, it sounds like moving from one filename format to the other would be a reasonable step in such a migration (and far from the most painful one).

@waldyrious
Copy link
Copy Markdown
Contributor Author

waldyrious commented Oct 2, 2019 •
edited
Loading

Add a new option to the first inner group, to support the test filenames currently used on the backend repo.

Is backend the only repo with such a format?

Ah, I forgot to add an update to that bit. It's not the only one. It was the only one I was aware of when I created this PR, but I ran a search in the repos I have cloned locally (not all of our repos!):

❯ find . -name '*_test.js' | grep -v node_modules | cut -d '/' -f2 | uniq

which returns 12 projects.

(I didn't check whether each of those uses Jest, though.)

That said, I do agree that we should converge to a single filename format, and have added to my personal list of tasks to work on (since we don't have a centralized place to track these issues...)

@waldyrious waldyrious force-pushed the support/improve-jest-testRegex branch from 1cc2084 to 667920f Compare October 2, 2019 10:57
@rplopes
Copy link
Copy Markdown
Contributor

rplopes commented Oct 2, 2019

From that list, at least http-errors uses jest.

Still, instead of allowing both formats, WDYT about renaming those files instead on projects as we add uphold-scripts to them? To push for convergence of standards.

@waldyrious
Copy link
Copy Markdown
Contributor Author

waldyrious commented Oct 2, 2019 •
edited
Loading

Still, instead of allowing both formats, WDYT about renaming those files instead on projects as we add uphold-scripts to them? To push for convergence of standards.

Sounds good to me. Just ran a quick check for the packages already using uphold-scripts and none of them are in the list above.

Will change.

@waldyrious
Improve testRegex in jest-preset.json
398da0e 
- Remove the "spec" part of the match,
  as none of the repos that use (or might use) uphold-scripts uses it
- Remove the unused regex groups
@waldyrious waldyrious force-pushed the support/improve-jest-testRegex branch from 667920f to 398da0e Compare October 2, 2019 11:37
@waldyrious
Copy link
Copy Markdown
Contributor Author

waldyrious commented Oct 2, 2019

Changes done. The regex is now just "\\.test\\.js$". The opening comment of the PR was also edited to match.

rplopes
rplopes approved these changes Oct 2, 2019
View reviewed changes
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.

LGTM

@rplopes rplopes merged commit e3eebe8 into master Oct 11, 2019
@rplopes rplopes deleted the support/improve-jest-testRegex branch October 11, 2019 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Americas Americas Americas left review comments

@ricardobcl ricardobcl Awaiting requested review from ricardobcl

+2 more reviewers

@NelsonBrandao NelsonBrandao NelsonBrandao left review comments

@rplopes rplopes rplopes approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@waldyrious waldyrious

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@waldyrious @rplopes @NelsonBrandao @Americas