Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize file names in integration-tests #5513

Merged
merged 3 commits into from Feb 14, 2018
Merged

Standardize file names in integration-tests #5513

merged 3 commits into from Feb 14, 2018

Conversation

ghost
Copy link

@ghost ghost commented Feb 10, 2018

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
Copy link
Member

cpojer commented Feb 10, 2018

Do you mind rebasing this and making sure CI passes?

@codecov-io
Copy link

codecov-io commented Feb 11, 2018

Codecov Report

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

Impacted file tree graph

@@           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
Copy link
Member

SimenB commented Feb 11, 2018

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

CHANGELOG.md Outdated
* `[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))
Copy link
Member

Choose a reason for hiding this comment

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

should be under master

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

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

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

CHANGELOG.md Outdated
* `[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))
Copy link
Author

Choose a reason for hiding this comment

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

updated changelog

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 | |
Copy link
Author

Choose a reason for hiding this comment

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

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

@@ -5,7 +5,7 @@ exports[`babel-jest instruments only specific files and collects coverage 1`] =
File | % Stmts | % Branch | % Funcs | % Lines |Uncovered Lines |
------------|----------|----------|----------|----------|----------------|
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 |
Copy link
Author

Choose a reason for hiding this comment

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

updated this snapshot since name changed

'<rootDir>/reporters/test_reporter.js',
{'Dmitrii Abramov': 'Awesome'},
],
['<rootDir>/reporters/TestReporter.js', {'Dmitrii Abramov': 'Awesome'}],
Copy link
Author

Choose a reason for hiding this comment

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

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

@ghost
Copy link
Author

ghost commented Feb 11, 2018

@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?

screen shot 2018-02-11 at 1 57 08 pm

@SimenB
Copy link
Member

SimenB commented Feb 11, 2018

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

@ghost
Copy link
Author

ghost commented Feb 11, 2018

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
Copy link
Author

ghost commented Feb 11, 2018

@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

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

Choose a reason for hiding this comment

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

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

Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

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

@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
Copy link
Member

SimenB commented Feb 12, 2018

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

@SimenB
Copy link
Member

SimenB commented Feb 13, 2018

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

@ghost
Copy link
Author

ghost commented Feb 13, 2018

@SimenB done 👍

@ghost
Copy link
Author

ghost commented Feb 14, 2018

@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 cpojer merged commit 10276f9 into jestjs:master Feb 14, 2018
@cpojer
Copy link
Member

cpojer commented Feb 14, 2018

@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
Copy link
Author

ghost commented Feb 14, 2018

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
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants