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

Add extra parameter: runTestsByPath. Fixes #4396 #4411

Merged
merged 1 commit into from
Sep 5, 2017
Merged

Add extra parameter: runTestsByPath. Fixes #4396 #4411

merged 1 commit into from
Sep 5, 2017

Conversation

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

@@ -162,6 +162,18 @@ class SearchSource {
};
}

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

Choose a reason for hiding this comment

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

nope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦🏻‍♂️

@@ -39,19 +39,18 @@ const getTestPaths = async (
) => {
const source = new SearchSource(context);
let data = await source.getTestPaths(globalConfig, changedFilesPromise);
if (!data.tests.length) {
Copy link
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
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
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
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
Member

cpojer commented Sep 1, 2017

I'll buy a test for 100 internet points.

Copy link
Contributor

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

@@ -162,6 +162,18 @@ class SearchSource {
};
}

findTests(paths: Array<Path>): SearchResult {
Copy link
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
Contributor Author

mjesun commented Sep 1, 2017

@cpojer that's called bitcoins nowadays

@mjesun
Copy link
Contributor Author

mjesun commented Sep 3, 2017

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

@cpojer
Copy link
Member

cpojer commented Sep 3, 2017

Tests still failing.

@codecov-io
Copy link

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

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