Skip to content

Add extra parameter: runTestsByPath. Fixes #4396#4411

Merged
cpojer merged 1 commit intojestjs:masterfrom
mjesun:add-run-tests-by-path
Sep 5, 2017
Merged

Add extra parameter: runTestsByPath. Fixes #4396#4411
cpojer merged 1 commit intojestjs:masterfrom
mjesun:add-run-tests-by-path

Conversation

@mjesun
Copy link
Copy Markdown
Contributor

@mjesun mjesun commented Sep 1, 2017

This option adds an extra runTestsByPath parameter into the CLI. The goal of the parameter is to be able to execute tests by directly giving their path. Since the non-prefixed arguments right now are understood as patterns for file paths, this kind of worked, but when having more than 5k tests, the matching is quadratic and then it becomes unfeasible.

Comment thread packages/jest-cli/src/search_source.js Outdated
}

findTests(paths: Array<Path>): SearchResult {
require('fs').writeFileSync('/tmp/lep', paths);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nope

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.

🤦🏻‍♂️

) => {
const source = new SearchSource(context);
let data = await source.getTestPaths(globalConfig, changedFilesPromise);
if (!data.tests.length) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lol how did that happen

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.

probably me refactoring it last time and giving up on reading all if/else conditions :D

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.

That's because this if is useless.

this._context,
paths
.map(p => path.resolve(process.cwd(), p))
.filter(this.isTestFilePath.bind(this)),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What happens if it can't find any tests, what does the message look like and does it make sense?

@cpojer
Copy link
Copy Markdown
Member

cpojer commented Sep 1, 2017

I'll buy a test for 100 internet points.

Copy link
Copy Markdown
Contributor

@boujeepossum boujeepossum left a comment

Choose a reason for hiding this comment

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

can you write an integration test for it?

Comment thread packages/jest-cli/src/search_source.js Outdated
};
}

findTests(paths: Array<Path>): SearchResult {
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.

can it be findTestsByPaths?

@mjesun
Copy link
Copy Markdown
Contributor Author

mjesun commented Sep 1, 2017

@cpojer that's called bitcoins nowadays

@mjesun
Copy link
Copy Markdown
Contributor Author

mjesun commented Sep 3, 2017

Changed name from findTests to findTestsByPath. Added a test to the integration_tests folder.

@cpojer
Copy link
Copy Markdown
Member

cpojer commented Sep 3, 2017

Tests still failing.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 4, 2017

Codecov Report

Merging #4411 into master will decrease coverage by 0.18%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4411      +/-   ##
==========================================
- Coverage   56.84%   56.66%   -0.19%     
==========================================
  Files         191      191              
  Lines        6475     6493      +18     
  Branches        5        5              
==========================================
- Hits         3681     3679       -2     
- Misses       2791     2811      +20     
  Partials        3        3
Impacted Files Coverage Δ
packages/jest-config/src/normalize.js 78.26% <ø> (ø) ⬆️
packages/jest-config/src/defaults.js 85.71% <ø> (ø) ⬆️
packages/jest-config/src/index.js 0% <ø> (ø) ⬆️
packages/jest-config/src/valid_config.js 100% <ø> (ø) ⬆️
packages/jest-cli/src/run_jest.js 0% <0%> (ø) ⬆️
packages/jest-cli/src/search_source.js 70.31% <0%> (-4.69%) ⬇️
...ackages/jest-cli/src/reporters/summary_reporter.js 10% <0%> (-2%) ⬇️
packages/jest-cli/src/get_no_test_found_verbose.js 0% <0%> (ø) ⬆️
packages/jest-cli/src/get_no_test_found.js 0% <0%> (ø) ⬆️
packages/jest-message-util/src/index.js 86.41% <0%> (-0.17%) ⬇️
... and 9 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 4398ccb...fdc152c. Read the comment docs.

@mjesun
Copy link
Copy Markdown
Contributor Author

mjesun commented Sep 5, 2017

@cpojer Looked at the error messages when failing and added an additional if so the feedback is better now.

@cpojer cpojer merged commit 2c910dc into jestjs:master Sep 5, 2017
tabrindle pushed a commit to tabrindle/jest that referenced this pull request Oct 2, 2017
@mjesun mjesun deleted the add-run-tests-by-path branch December 6, 2017 11:47
@github-actions
Copy link
Copy Markdown

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 13, 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.

5 participants