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: Fix integration folder names #5298

Merged
merged 2 commits into from
Jan 14, 2018

Conversation

jamischarles
Copy link
Contributor

Summary

This is the first of many PRs to standardize file names and fix #4969. Per @thymikee guidance I'll be making these changes incrementally. This PR only renames folder names. A future PR will include filenames.

In this PR I:

  • ran mv integration_tests integration-tests
  • renamed all the subfolders under integration-tests

Now the folders (not filenames yet) follow standard Facebook 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.

List of commands I ran for all subfolders:

  • find . -name '*.js' -print0 | xargs -0 sed -i "" "s/integration_tests/integration-tests/g" I ran this command across .md, .snap, .json, .gitignore files

Folders I left untouched

  • __mocks__, node_modules, __tests__, basically anything with a leading __

All the test cases pass. I'm happy to squash the commits as well. Any feedback?

@codecov-io
Copy link

codecov-io commented Jan 13, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5298   +/-   ##
=======================================
  Coverage   61.22%   61.22%           
=======================================
  Files         205      205           
  Lines        6894     6894           
  Branches        4        4           
=======================================
  Hits         4221     4221           
  Misses       2672     2672           
  Partials        1        1
Impacted Files Coverage Δ
...bel-plugin-jest-hoist/__test_modules__/Unmocked.js 100% <ø> (ø)
...ests/babel-plugin-jest-hoist/__test_modules__/d.js 100% <ø> (ø)
integration-tests/coverage-remapping/covered.ts 85.71% <ø> (ø)
...ests/babel-plugin-jest-hoist/__test_modules__/b.js 0% <ø> (ø)
...s/coverage-report/cached-duplicates/a/identical.js 100% <ø> (ø)
...ests/babel-plugin-jest-hoist/__test_modules__/c.js 100% <ø> (ø)
...ests/babel-plugin-jest-hoist/__test_modules__/a.js 0% <ø> (ø)
...gration-tests/babel-plugin-jest-hoist/mock-file.js 100% <ø> (ø)
...babel-plugin-jest-hoist/__test_modules__/Mocked.js 0% <ø> (ø)
...s/coverage-report/cached-duplicates/b/identical.js 100% <ø> (ø)
... and 4 more

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 c799a02...87b3db4. Read the comment docs.

CHANGELOG.md Outdated
### Chore & Maintenance

* `[filenames]` Standardize folder names under `integration-tests/`
([#x](https://github.com/facebook/jest/pull/x))
Copy link
Collaborator

Choose a reason for hiding this comment

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

x = 5298 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Looks good, small nits noted

@@ -13,14 +13,14 @@ const path = require('path');
const runJest = require('../runJest');
const {cleanup} = require('../utils');

const DIR = path.join(os.tmpdir(), 'jest_global_setup');
const DIR = path.join(os.tmpdir(), 'jest_global-setup');
Copy link
Collaborator

Choose a reason for hiding this comment

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

In multiple places this need to be changed to: jest-global-setup (and jest-global-teardown accordingly too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right you are! Searched and fixed these files:

→ git status -s                                                                                                            
 M integration-tests/__tests__/global_setup.test.js
 M integration-tests/__tests__/global_teardown.test.js
 M integration-tests/global-setup/__tests__/setup1.test.js
 M integration-tests/global-setup/__tests__/setup2.test.js
 M integration-tests/global-setup/__tests__/setup3.test.js
 M integration-tests/global-setup/setup.js
 M integration-tests/global-teardown/__tests__/teardown1.test.js
 M integration-tests/global-teardown/__tests__/teardown2.test.js
 M integration-tests/global-teardown/__tests__/teardown3.test.js
 M integration-tests/global-teardown/teardown.js

@jamischarles
Copy link
Contributor Author

@thymikee Both nits addressed.

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Looks like these 2 changes are the last for this PR :D

@@ -13,19 +13,19 @@ const os = require('os');
const path = require('path');
const runJest = require('../runJest');

const CACHE = path.resolve(os.tmpdir(), 'clear_cache_directory');
const CACHE = path.resolve(os.tmpdir(), 'clear-cache_directory');
Copy link
Collaborator

Choose a reason for hiding this comment

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

clear-cache-directory

@@ -14,7 +14,7 @@ const runJest = require('../runJest');
const os = require('os');
const path = require('path');

const DIR = path.resolve(os.tmpdir(), 'custom_reporters_test_dir');
const DIR = path.resolve(os.tmpdir(), 'custom-reporters_test_dir');
Copy link
Collaborator

Choose a reason for hiding this comment

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

custom-reporters-test-dir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes. Sorry about that. Both of these are fixed.

Fixes all the folder names in the integration-tests folder to follow Facebook internal file / folder
naming conventions. This is the first of several incremental PRs.
Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Looks like it, thank you!

@cpojer cpojer merged commit 0580b5b into jestjs:master Jan 14, 2018
@jamischarles
Copy link
Contributor Author

🎉 thanks.

@thymikee thank you for all your help. Next PR should come next Friday.

@thymikee
Copy link
Collaborator

You're welcome!

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

Standardize file naming
5 participants