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

Rename DanTup_tiler.test to DanTup_tiler.test.disabled #376

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

zanderso
Copy link
Member

@zanderso zanderso commented Jun 7, 2024

The pixel tests in the tiler suite are experiencing the same failure as is described in flutter/flutter#149934

cc @DanTup

| ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
| The following assertion was thrown while running async test code:
| Golden "test-maps/geometry/orthogonal_macos_arm.png": Pixel test failed, 0.00%, 8px diff detected.
| Failure feedback can be found at
| /Volumes/Work/s/w/ir/x/t/flutter_customer_testing.DanTup_tiler.GmgOoy/tests/test/failures
|
| When the exception was thrown, this was the stack:
| #0      LocalFileComparator.compare (package:flutter_test/src/_goldens_io.dart:106:5)
| <asynchronous suspension>
| #1      MatchesGoldenFile.matchAsync.<anonymous closure> (package:flutter_test/src/_matchers_io.dart:118:32)
| <asynchronous suspension>
| <asynchronous suspension>
| #3      _expect.<anonymous closure> (package:matcher/src/expect/expect.dart:123:26)
| <asynchronous suspension>
| <asynchronous suspension>
| #5      expectLater.<anonymous closure> (package:flutter_test/src/widget_tester.dart:512:24)
| <asynchronous suspension>
| #6      expectMapRender (file:///Volumes/Work/s/w/ir/x/t/flutter_customer_testing.DanTup_tiler.GmgOoy/tests/test/utils.dart:43:5)
| <asynchronous suspension>
| #7      testMapRender.<anonymous closure> (file:///Volumes/Work/s/w/ir/x/t/flutter_customer_testing.DanTup_tiler.GmgOoy/tests/test/utils.dart:20:5)
| <asynchronous suspension>
| #8      testWidgets.<anonymous closure>.<anonymous closure> (package:flutter_test/src/widget_tester.dart:183:15)
| <asynchronous suspension>
| #9      TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:1025:5)
| <asynchronous suspension>
| <asynchronous suspension>
| (elided 3 frames from dart:async and package:stack_trace)
|
| The exception was caught asynchronously.
| ════════════════════════════════════════════════════════════════════════════════════════════════════
|
| 00:05 +10 -1: geometry/orthogonal [E]
|   Test failed. See exception logs above.
|   The test description was: geometry/orthogonal
|
|
| To run this test again: /Volumes/Work/s/w/ir/x/w/flutter/bin/cache/dart-sdk/bin/dart test /Volumes/Work/s/w/ir/x/t/flutter_customer_testing.DanTup_tiler.GmgOoy/tests/test/render_test.dart -p vm --plain-name 'geometry/orthogonal'

The pixel tests in the tiler suite are experiencing the same failure as is described in flutter/flutter#149934
@zanderso zanderso requested a review from matanlurey June 7, 2024 22:09
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@zanderso zanderso merged commit 8a6d8b5 into main Jun 7, 2024
12 checks passed
@zanderso zanderso deleted the zanderso-patch-1 branch June 7, 2024 22:27
DanTup added a commit to DanTup/tiler-test-maps that referenced this pull request Jun 8, 2024
DanTup added a commit to DanTup/tiler that referenced this pull request Jun 8, 2024
@DanTup
Copy link
Contributor

DanTup commented Jun 8, 2024

@zanderso I updated the goldens in that repo for the latest Flutter. I don't know how useful these tests are (I'm not developing/maintaining Tiler besides fixing these tests).. If you think they're useful I can send a PR to re-enable this and update the hash, otherwise they could be removed.

@DanTup
Copy link
Contributor

DanTup commented Jun 25, 2024

@zanderso any opinions on the above? Should I file a PR to re-enable these now they've updated, or should we remove it? I don't know if I'm providing any tests the framework doesn't already have (I think so far all cases of failures here have been known/expected changes and result in this dance). I'm happy to maintain the tests if there's some value in them though.

@zanderso
Copy link
Member Author

Are there any tests aside from the golden tests in the tiler suite? The golden tests are probably not high-value, and we should delete those. If there are non-golden tests for tiler, then they probably have a better cost/benefit trade-off, and we should keep them.

@DanTup
Copy link
Contributor

DanTup commented Jun 25, 2024

Nope, no other tests. The tests were all specifically for rendering images/sprites/shapes onto a canvas. If those are things are already well-tested by Flutter and there are enough other projects here to ensure we feel the pain users feel when the goldens change in breaking ways, I agree it probably makes sense to remove them. I'll open a PR.

DanTup added a commit to DanTup/tests that referenced this pull request Jun 25, 2024
I haven't been maintaining this package for a while (besides updating tests). Often subtle (acceptable) changes to the golden files cause a break that requires updating multiple repositories (tests, Tiler, Tiler-test-maps) to unblock rolls.

Maintaining these tests is probably not a good trade-off given they're really only testing things that Flutter should already be testing (rendering some images and shapes to a canvas).

See flutter#376
auto-submit bot pushed a commit that referenced this pull request Jun 25, 2024
I haven't been maintaining this package for a while (besides updating tests). Often subtle (acceptable) changes to the golden files cause a break that requires updating multiple repositories (tests, Tiler, Tiler-test-maps) to unblock rolls.

Maintaining these tests is probably not a good trade-off given they're really only testing things that Flutter should already be testing (rendering some images and shapes to a canvas).

See #376

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

4 participants