Skip to content

Conversation

CillianMyles
Copy link
Collaborator

@CillianMyles CillianMyles commented Jul 24, 2024

This PR makes the goldens pass by:

  • Addressing observed strange mix of Android and iOS configurations in super_textfield_toolbar_test.dart (although admittedly I do not understand the original intention by this mixing)
  • Regenerating them from the golden tests docker image

@matthew-carroll
Copy link
Contributor

@angelosilvestre can you look at these changes and see if you understand what's going on?

@CillianMyles - You might get goldens to work from a Linux machine by chance, but technically we use the Docker-based runner described in the docs. We should get goldens working with that.

Also, I see that this PR changed the platform target of some tests. My initial thoughts is that that's not an appropriate "fix" because that's altering the actual thing under test.

@matthew-carroll
Copy link
Contributor

@CillianMyles please let me know if the golden infrastructure is too difficult to use based on the docs I linked in the other PR, or if you're otherwise unable to get them generating. We'll wanna get this corrected as soon as possible given that the error is reported in every PR.

@CillianMyles
Copy link
Collaborator Author

@matthew-carroll I was able to get docker and the setup for golden tests running, and successfully regenerated the goldens in both this PR, and option 2 (#2184).

I also updated the PR description for both, to help explain the difference in approach.

Please let me know if either of these PR's are okay for you, or if we should take another route. Admittedly I don't know why the goldens needed to be regenerated, but not something I had too much time spend digging into, I mostly wanted to clear the way so my other PR (#2139) can land.

I also created a PR to update the docs with some more info on installing docker (#2199).

@CillianMyles CillianMyles deleted the maintenance/fix-goldens-1 branch July 27, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants