Standardize file names in integration-tests · Pull Request #5513 · jestjs/jest

ghost · 2018-02-10T02:02:20Z

Summary

More work on #4969.

fix file names in integration-tests folders and sub-folders only

Details

ran git mv to rename files
used Atom's search and replace to replace all occurrences of a file renaming with the new filename
updated jest snapshots when failing because changed file names

Now files in root can follow standard FB practice:

Files that primarily export types, objects or classes should use CapitalizedFileNames.js and should mirror what’s inside 1:1.
Files that export a single function should have the function name with camelCase in it.
Folder names should use dashes, unless they are special folders.

Test plan

Relying on yarn test and CI tests to pass.

Let me know if I've missed something.

cpojer · 2018-02-10T10:49:41Z

Do you mind rebasing this and making sure CI passes?

codecov-io · 2018-02-11T18:46:19Z

Codecov Report

Merging #5513 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5513   +/-   ##
=======================================
  Coverage   61.67%   61.67%           
=======================================
  Files         213      213           
  Lines        7160     7160           
  Branches        4        3    -1     
=======================================
  Hits         4416     4416           
  Misses       2743     2743           
  Partials        1        1

Impacted Files	Coverage Δ
...rm/babel-jest/this-directory-is-covered/Covered.js	`83.33% <ø> (ø)`
...s/coverage-report/cached-duplicates/b/Identical.js	`100% <ø> (ø)`
...s/coverage-report/cached-duplicates/a/Identical.js	`100% <ø> (ø)`
packages/jest-config/src/valid_config.js	`100% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ca959b...de423c8. Read the comment docs.

SimenB · 2018-02-11T18:51:41Z

Mind fixing the unicorn/filename-case lint warnings from #5500?

SimenB · 2018-02-11T18:52:01Z

-
 * `[filenames]` Standardize file names in root
  ([#5500](https://github.com/facebook/jest/pull/5500))
+* `[filenames]` Standardize files names in "integration-tests" folder ([#5513](https://github.com/facebook/jest/pull/5513))


should be under master

good catch! didn't realize it was below a release number. I'll fix

ghost

explaining a few changes that came about in this pr that aren't as clear as renaming a file or changing an import

ghost · 2018-02-11T18:46:37Z

-
 * `[filenames]` Standardize file names in root
  ([#5500](https://github.com/facebook/jest/pull/5500))
+* `[filenames]` Standardize files names in "integration-tests" folder ([#5513](https://github.com/facebook/jest/pull/5513))


updated changelog

ghost · 2018-02-11T18:47:41Z

-  other-file.js                      |      100 |      100 |      100 |      100 |                |
-  sum.js                             |    85.71 |      100 |       50 |    85.71 |             12 |
-  sum_dependency.js                  |        0 |        0 |        0 |        0 |     8,10,11,14 |
+  OtherFile.js                       |      100 |      100 |      100 |      100 |                |


updated this snapshot because of file names changing, but if you look, you can see no number changed 👍

ghost · 2018-02-11T18:48:00Z

 ------------|----------|----------|----------|----------|----------------|
 All files   |    83.33 |      100 |       50 |    83.33 |                |
- covered.js |    83.33 |      100 |       50 |    83.33 |             15 |
+ Covered.js |    83.33 |      100 |       50 |    83.33 |             15 |


updated this snapshot since name changed

ghost · 2018-02-11T18:48:53Z

-          '<rootDir>/reporters/test_reporter.js',
-          {'Dmitrii Abramov': 'Awesome'},
-        ],
+        ['<rootDir>/reporters/TestReporter.js', {'Dmitrii Abramov': 'Awesome'}],


prettier was failing on this, so i changed it until atom didn't complain

ghost · 2018-02-11T18:58:36Z

@SimenB for the lint warnings for the filenames, I thought we don't want snake case anymore. So what's the suggested path for fixing these?

SimenB · 2018-02-11T19:02:24Z

The fix for lint is to change the rule to match the new convention 🙂

ghost · 2018-02-11T19:14:16Z

Ok so the problem with changing the lint rule is that the convention wants to support multiple styles (camelCase and pascalCase) and the lint rule only allows one style (as far as i understand).

Lint Rule

Here's how to change the lint rule: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/docs/rules/filename-case.md

File Naming Conventions

Here's the file naming conventions as I understand them:

#4969 (comment)

Files that primarily export types, objects or classes should use CapitalizedFileNames.js and should mirror what’s inside 1:1. (pascalCase)
Files that export a single function should have the function name with camelCase in it. (camelCase)
Folder names should use dashes, unless they are special folders.

ghost · 2018-02-11T19:18:02Z

@SimenB could be wrong but looks like the lint rule only allows one case to be defined for file names: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/b7e4afbf6562b93d88f6c652a64df72316c73004/rules/filename-case.js#L101

ghost · 2018-02-12T22:10:59Z

+File      |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
+----------|----------|----------|----------|----------|----------------|
+All files |        0 |      100 |      100 |        0 |                |
+ setup.js |        0 |      100 |      100 |        0 |              8 |


actually, this snapshot is now missing OtherFile after renaming it from other-file to OtherFile

ghost

@cpojer @SimenB updated again since conflicts came up. Please let me know what you think about the changes.

I don't think we should do the lint rule in this pr given my above comments about multiple styles not being supported.

SimenB · 2018-02-12T22:33:57Z

If multiple styles are not supported, then we should remove the rule, I think

SimenB · 2018-02-13T07:45:58Z

Can you remove unicorn completely? We only used it for this single rule

ghost · 2018-02-13T14:31:23Z

@SimenB done 👍

- updated tests - added to changelog

ghost · 2018-02-14T16:05:38Z

@SimenB just fixed some more conflicts. Since this pr touches so many of the integration tests, can you give another review? I'm starting to get tired of keeping this up to date

cpojer · 2018-02-14T19:08:57Z

@baldwmic thanks for sticking with us. The Jest project moves fast and lots of people contribute stuff to it, sorry it took a while to get this merged.

ghost · 2018-02-14T19:14:06Z

thank you @SimenB for the reviews and @cpojer for making contributing a good experience! 🎉

not trying to nag but figured i'd let you know i was burning out of keeping the pr up to date. regardless, it's in 😸

github-actions · 2021-05-12T20:07:23Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

facebook-github-bot added the CLA Signed ✔️ label Feb 10, 2018

SimenB reviewed Feb 11, 2018

View reviewed changes

ghost commented Feb 11, 2018

View reviewed changes

ghost commented Feb 12, 2018

View reviewed changes

Michael Baldwin added 3 commits February 14, 2018 10:43

standardize file names in integration-tests

435298d

- updated tests - added to changelog

remove filename lint rule since multiple styles are not supported

4905c97

remove unicorn completely

de423c8

SimenB approved these changes Feb 14, 2018

View reviewed changes

cpojer merged commit 10276f9 into jestjs:master Feb 14, 2018

renovate Bot mentioned this pull request Feb 14, 2018

fix(deps): update dependency jest to ^22.0.0 manekinekko/devoxx-assistant#3

Open

github-actions Bot locked as resolved and limited conversation to collaborators May 12, 2021

Uh oh!

Conversation

ghost commented Feb 10, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

Test plan

Uh oh!

cpojer commented Feb 10, 2018

Uh oh!

codecov-io commented Feb 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

SimenB commented Feb 11, 2018

Uh oh!

SimenB Feb 11, 2018

Choose a reason for hiding this comment

Uh oh!

ghost Feb 11, 2018

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost Feb 11, 2018

Choose a reason for hiding this comment

Uh oh!

ghost Feb 11, 2018

Choose a reason for hiding this comment

Uh oh!

ghost Feb 11, 2018

Choose a reason for hiding this comment

Uh oh!

ghost Feb 11, 2018

Choose a reason for hiding this comment

Uh oh!

ghost commented Feb 11, 2018

Uh oh!

SimenB commented Feb 11, 2018

Uh oh!

ghost commented Feb 11, 2018

Lint Rule

File Naming Conventions

Uh oh!

ghost commented Feb 11, 2018

Uh oh!

ghost Feb 12, 2018

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

SimenB commented Feb 12, 2018

Uh oh!

SimenB commented Feb 13, 2018

Uh oh!

ghost commented Feb 13, 2018

Uh oh!

ghost commented Feb 14, 2018

Uh oh!

cpojer commented Feb 14, 2018

Uh oh!

ghost commented Feb 14, 2018

Uh oh!

github-actions Bot commented May 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ghost commented Feb 10, 2018 •

edited by ghost

Loading

codecov-io commented Feb 11, 2018 •

edited

Loading