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

[Dialogs] Try to fix testActionsAreOrderedByEmphasis snapshot #9862

Merged

Conversation

andrewoverton
Copy link
Contributor

I just tried checking out develop and I got this, which I've seen many times before:

Encountered 1 file(s) that should have been pointers, but weren't:
snapshot_test_goldens/goldens_64/MDCAlertControllerActionsTests/testActionsAreOrderedByEmphasis_11_2@2x.png

I tried regenerating the snapshot and looking at the comparison on GitHub. I couldn't preview the snapshot. I can't remember if you're supposed to be able to preview the snapshot after replacing a binary file with a Git LFS file...

I looked at the PR the problematic snapshot was added in and saw that I was able to preview it there. I wasn't expecting this, because up to this point I thought that if you could preview the snapshot in the PR that meant everything was good. I guess not? It said it was a binary file, and not a git LFS file. I also noticed the PR was exported from an internal CL. So I guess if you export a CL to a PR and it has a binary snapshot you can preview it, but that doesn't mean it should be merged.

I tried issuing the following command to fix it:

git lfs migrate import --no-rewrite snapshot_test_goldens/goldens_64/MDCAlertControllerActionsTests/testActionsAreOrderedByEmphasis_11_2@2x.png

That resulted in 224a322. The commit message said it converted the binary file to a Git LFS file, which is good. However, I still couldn't preview it, and I'm used to using that as a test to make sure I've done it correctly, for better or worse. I decided to just rename the test and generate a new snapshot. That's in 10b7b37. Now GitHub says it's a Git LFS file, and I can preview it, so I think this should solve the issue.

@andrewoverton andrewoverton requested review from galiak11, iberezan and a team as code owners March 9, 2020 05:10
@material-automation material-automation bot changed the title Try to fix testActionsAreOrderedByEmphasis snapshot [Dialogs] Try to fix testActionsAreOrderedByEmphasis snapshot Mar 9, 2020
@material-automation
Copy link

Based on the changes, the title has been prefixed with [Dialogs].

@material-automation
Copy link

material-automation commented Mar 9, 2020

bazel detected changes to the following targets:

//components/Dialogs:snapshot_tests
//components/Dialogs:snapshot_tests_test_bundle

@andrewoverton andrewoverton merged commit 1f0f035 into material-components:develop Mar 9, 2020
@andrewoverton andrewoverton deleted the fix-snapshot branch March 9, 2020 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants