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

ui tests: diff from old (expected) to new (actual) instead of backwards. #47978

Merged
merged 1 commit into from
Feb 5, 2018

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Feb 3, 2018

Previously actual was "old" and expected was "new" which resulted in + before -.
AFAIK all diff tools put - before +, which made the previous behavior very confusing.

r? @nikomatsakis

@eddyb
Copy link
Member Author

eddyb commented Feb 3, 2018

Looks like this has already been fixed once, by #42144, but recently regressed by #47185.
cc @cengizio @ritiek (I assume/hope the regression isn't intentional?)

@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 3, 2018
@kennytm
Copy link
Member

kennytm commented Feb 3, 2018

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 3, 2018

📌 Commit cc68afb has been approved by kennytm

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 3, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Feb 3, 2018
ui tests: diff from old (expected) to new (actual) instead of backwards.

Previously `actual` was "old" and `expected` was "new" which resulted in `+` before `-`.
AFAIK all diff tools put `-` before `+`, which made the previous behavior *very confusing*.

r? @nikomatsakis
@ritiek
Copy link
Member

ritiek commented Feb 3, 2018

Yeah, I left that intentionally since it was somehow causing problems with the way diff was being shown. Apologizes, I didn't notice that '+' was being shown before '-'.

bors added a commit that referenced this pull request Feb 3, 2018
Rollup of 9 pull requests

- Successful merges: #47753, #47862, #47877, #47896, #47912, #47944, #47947, #47978, #47958
- Failed merges:
kennytm added a commit to kennytm/rust that referenced this pull request Feb 4, 2018
ui tests: diff from old (expected) to new (actual) instead of backwards.

Previously `actual` was "old" and `expected` was "new" which resulted in `+` before `-`.
AFAIK all diff tools put `-` before `+`, which made the previous behavior *very confusing*.

r? @nikomatsakis
kennytm added a commit to kennytm/rust that referenced this pull request Feb 4, 2018
ui tests: diff from old (expected) to new (actual) instead of backwards.

Previously `actual` was "old" and `expected` was "new" which resulted in `+` before `-`.
AFAIK all diff tools put `-` before `+`, which made the previous behavior *very confusing*.

r? @nikomatsakis
bors added a commit that referenced this pull request Feb 4, 2018
Rollup of 10 pull requests

- Successful merges: #47862, #47877, #47896, #47912, #47947, #47958, #47978, #47996, #47999, #47892
- Failed merges:
bors added a commit that referenced this pull request Feb 4, 2018
Rollup of 10 pull requests

- Successful merges: #47862, #47877, #47896, #47912, #47947, #47958, #47978, #47996, #47999, #47892
- Failed merges:
@bors bors merged commit cc68afb into rust-lang:master Feb 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants