Skip to content

[google_maps_web] Skip a smaller subset of web tests #7087

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.

Sign up for GitHub

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

Closed
bparrishMines wants to merge 9 commits into from
Closed

[google_maps_web] Skip a smaller subset of web tests #7087

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
52fe074
undo web int test skip
bparrishMines Jul 9, 2024
6d963d8
Merge branch 'main' of github.com:flutter/packages into gmf_web_fix
bparrishMines Jul 10, 2024
408fcff
skip just markers test
bparrishMines Jul 10, 2024
a814823
skip if web
bparrishMines Jul 10, 2024
0d0647b
skip specific tests
bparrishMines Jul 11, 2024
fce6680
Merge branch 'main' of github.com:flutter/packages into gmf_web_fix
bparrishMines Jul 11, 2024
f7f189b
Merge branch 'main' of github.com:flutter/packages into gmf_web_fix
bparrishMines Jul 15, 2024
ac6c7b2
Merge branch 'main' of github.com:flutter/packages into gmf_web_fix
bparrishMines Jul 16, 2024
75a61a2
Merge branch 'main' of github.com:flutter/packages into gmf_web_fix
bparrishMines Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ...es/google_maps_flutter/google_maps_flutter/example/integration_test/google_maps_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'package:integration_test/integration_test.dart';

import 'src/maps_controller.dart' as maps_controller;
import 'src/maps_inspector.dart' as maps_inspector;
import 'src/shared.dart';
import 'src/tiles_inspector.dart' as tiles_inspector;

/// Recombine all test files in `src` into a single test app.
Expand All @@ -15,6 +16,12 @@ import 'src/tiles_inspector.dart' as tiles_inspector;
void main() {
IntegrationTestWidgetsFlutterBinding.ensureInitialized();

// TODO(bparrishMines): These tests are failing on web due to an error being
// thrown after completion. See https://github.com/flutter/flutter/issues/145149
if (isWeb) {
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Jul 17, 2024

Choose a reason for hiding this comment

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

I know it'll be a lot more code, but I think it would be better to do something like:

// TODO(bparrishMines): Tests are failing on web due to an error being
// thrown after completion. See https://github.com/flutter/flutter/issues/145149
const bool _skipOnWeb = isWeb;

[ Add skip: _skipOnWeb to every test ]

That way they it'll be obvious both when looking at the test, and from the test run output, that web is skipping tests, vs them being silently skipped in an unusual place.

return;
}

maps_controller.runTests();
maps_inspector.runTests();
tiles_inspector.runTests();
Expand Down
5 changes: 4 additions & 1 deletion ...es/google_maps_flutter/google_maps_flutter_web/example/integration_test/markers_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,5 +497,8 @@ void main() {
expect(event, isA<InfoWindowTapEvent>());
expect((event as InfoWindowTapEvent).value, equals(const MarkerId('1')));
});
});
},
// TODO(bparrishMines): This is failing due to an error being thrown after
// completion. See https://github.com/flutter/flutter/issues/145149
skip: true);
}
2 changes: 0 additions & 2 deletions script/configs/exclude_integration_web.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,2 @@
# Currently missing: https://github.com/flutter/flutter/issues/82211
- file_selector
# Waiting on https://github.com/flutter/flutter/issues/145149
- google_maps_flutter
Comment on lines -3 to -4
Copy link
Member

@ditman ditman Aug 1, 2024

Choose a reason for hiding this comment

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

Yikes! is this also preventing google_maps_flutter_web from running on CI?

Copy link
Member

@ditman ditman Aug 1, 2024

Choose a reason for hiding this comment

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

It does:

============================================================
|| Not running for packages/google_maps_flutter/google_maps_flutter_web; excluded [@7:02]
============================================================

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Aug 1, 2024

Choose a reason for hiding this comment

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

I believe they were both failing the same way.

Copy link
Member

@ditman ditman Aug 1, 2024

Choose a reason for hiding this comment

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

I have a "fix" for _web at least:

(The root cause seems to be a bug in the html renderer not letting us adjust the devicePixelRatio)

For the web, I just stripped the DPR overrides from the tests (for now), but in cross-platform tests the DPR overrides probably make more sense.

Copy link
Member

@ditman ditman Aug 1, 2024

Choose a reason for hiding this comment

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

I think the only tests that need to be disabled are those that set the tester.view.devicePixelRatio AND are running with the html renderer (CHANNEL=stable). Canvaskit seems to not be affected.

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Aug 1, 2024

Choose a reason for hiding this comment

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

Would they pass if if we just wrapped the call to set devicePixelRatio in an if (!kIsWeb) (with a TODO to remove that condition later)?