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 message when changedSince is being used #7028

Merged
merged 5 commits into from
Sep 23, 2018

Conversation

natealcedo
Copy link

Summary

This pull request makes a quality of life improvement as referred here (#7026). If a changedSince flag is being used, the test runner should update the message being shown to reflect the ref that changedSince is being pointed to. This PR also updates the runner to be aware of this new message format.

Test plan

I've added tests cases in https://github.com/facebook/jest/blob/master/e2e/__tests__/only_changed.test.js and https://github.com/facebook/jest/blob/master/packages/jest-editor-support/src/__tests__/runner.test.js to reflect the changes.

@natealcedo natealcedo changed the title Fix message with changed since Fix message when changedSince is being used Sep 23, 2018
@codecov-io
Copy link

Codecov Report

Merging #7028 into master will decrease coverage by <.01%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7028      +/-   ##
==========================================
- Coverage   66.95%   66.95%   -0.01%     
==========================================
  Files         250      250              
  Lines       10428    10430       +2     
  Branches        4        3       -1     
==========================================
+ Hits         6982     6983       +1     
- Misses       3445     3446       +1     
  Partials        1        1
Impacted Files Coverage Δ
...est-cli/src/getNoTestFoundRelatedToChangedFiles.js 0% <0%> (ø) ⬆️
packages/jest-editor-support/src/Runner.js 74.25% <100%> (+0.25%) ⬆️

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 0418cab...2f4c3bd. Read the comment docs.

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.

This is perfect, thank you so much!

@@ -5,7 +5,7 @@
- `[pretty-format]` Option to not escape strings in diff messages ([#5661](https://github.com/facebook/jest/pull/5661))
- `[jest-haste-map]` Add `getFileIterator` to `HasteFS` for faster file iteration ([#7010](https://github.com/facebook/jest/pull/7010)).
- `[jest-worker]` [**BREAKING**] Add functionality to call a `setup` method in the worker before the first call and a `teardown` method when ending the farm ([#7014](https://github.com/facebook/jest/pull/7014)).
- `[jest-config]` [**BREAKING**] Set default `notifyMode` to `faluire-change` ([#7024](https://github.com/facebook/jest/pull/7024))
- `[jest-config]` [**BREAKING**] Set default `notifyMode` to `failure-change` ([#7024](https://github.com/facebook/jest/pull/7024))
Copy link
Member

Choose a reason for hiding this comment

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

😅

@SimenB SimenB merged commit df78b49 into jestjs:master Sep 23, 2018
@natealcedo natealcedo deleted the fix_message_with_changedSince branch September 23, 2018 06:45
@natealcedo
Copy link
Author

Sweet. What a prompt response! 👍

@@ -177,10 +177,10 @@ export default class Runner extends EventEmitter {

findMessageType(buf: Buffer): MessageType {
const str = buf.toString('utf8', 0, 58);
if (str === 'No tests found related to files changed since last commit.') {
const lastCommitRegex = /No tests found related to files changed since ((last commit)|([a-z0-9]+))./;
Copy link
Author

@natealcedo natealcedo Sep 23, 2018

Choose a reason for hiding this comment

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

@SimenB I just realised that this regular expression should include the quotation marks. The reason why the tests passed was because the tests were wrong. Going to make a new PR to fix this.

Edit: #7029 should fix this small issue.

@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