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

Fix unhandled error when a bad revision is provided to changedSince #7112

Closed
wants to merge 4 commits into from
Closed

Fix unhandled error when a bad revision is provided to changedSince #7112

wants to merge 4 commits into from

Conversation

khuyn003
Copy link
Contributor

@khuyn003 khuyn003 commented Oct 6, 2018

Summary

This PR fixes #7025 – when a bad revision is passed to the --changedSince argument, causing an unhandled rejection error message. It will now output the error message from git/hg instead.

Before:
image

After
image

Test plan

Updated integration tests.

@khuyn003 khuyn003 changed the title Changed since bug Fix unhandled error when a bad revision is provided to changedSince Oct 6, 2018
@codecov-io
Copy link

Codecov Report

Merging #7112 into master will decrease coverage by 0.05%.
The diff coverage is 27.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7112      +/-   ##
==========================================
- Coverage   66.67%   66.62%   -0.06%     
==========================================
  Files         253      253              
  Lines       10597    10607      +10     
  Branches        3        4       +1     
==========================================
+ Hits         7066     7067       +1     
- Misses       3530     3539       +9     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-changed-files/src/hg.js 28% <0%> (-7%) ⬇️
packages/jest-changed-files/src/git.js 84.61% <55.55%> (-15.39%) ⬇️

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 d59a4d3...e1ee284. Read the comment docs.


console.error(`\n\n${errorMsg}\n`);

process.exit(0);
Copy link
Member

Choose a reason for hiding this comment

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

it should exit with code 1, not 0


process.exit(0);

return [];
Copy link
Member

Choose a reason for hiding this comment

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

do we need to return after the process.exit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's mainly to pass the flow tests, though eslint also complained.

.filter(s => s !== '')
.map(changedPath => path.resolve(cwd, changedPath));
} catch (e) {
const errorMsg = e.message.split('\n')[1];
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this! I left some inline comments 🙂

@SimenB
Copy link
Member

SimenB commented Oct 7, 2018

Oh, another thing. What do yu think about wrapping the invocations of these functions in https://github.com/facebook/jest/blob/8e6b34520517033be0cbd866ccd70b91cb237696/packages/jest-changed-files/src/index.js#L32-L48? Then it's enough to just change there instead of the same change in both git and hg.

EDIT: Even better might be to wrap here: https://github.com/facebook/jest/blob/8c86fe8aecfab1a8815c804f9d1dd539a7fe72f6/packages/jest-cli/src/getChangedFilesPromise.js#L23-L27

Then it's in jest-cli and we don't have to add a dep to jest-message-util in jest-changed-files

@khuyn003
Copy link
Contributor Author

khuyn003 commented Oct 7, 2018

@SimenB Closing this and created a new PR (#7115)

@khuyn003 khuyn003 closed this Oct 7, 2018
@khuyn003
Copy link
Contributor Author

khuyn003 commented Oct 7, 2018

Oh, another thing. What do yu think about wrapping the invocations of these functions in

jest/packages/jest-changed-files/src/index.js

Lines 32 to 48 in 8e6b345

const gitPromises = Array.from(repos.git).map(repo =>
git.findChangedFiles(repo, changedFilesOptions),
);

const hgPromises = Array.from(repos.hg).map(repo =>
hg.findChangedFiles(repo, changedFilesOptions),
);

const changedFiles = (await Promise.all(
gitPromises.concat(hgPromises),
)).reduce((allFiles, changedFilesInTheRepo) => {
for (const file of changedFilesInTheRepo) {
allFiles.add(file);
}

return allFiles;
}, new Set());
? Then it's enough to just change there instead of the same change in both git and hg.
EDIT: Even better might be to wrap here:

jest/packages/jest-cli/src/getChangedFilesPromise.js

Lines 23 to 27 in 8c86fe8

return getChangedFilesForRoots(allRootsForAllProjects, {
changedSince: globalConfig.changedSince,
lastCommit: globalConfig.lastCommit,
withAncestor: globalConfig.changedFilesWithAncestor,
});
Then it's in jest-cli and we don't have to add a dep to jest-message-util in jest-changed-files

Not exactly sure why, but wrapping l23-l27 w/ a try/catch didn't work – the error gets thrown anyway and it never catches. I went with the above suggestion to avoid the same changes in both hg.js and git.js.

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

jest --changedSince=blablabla crashes
4 participants