Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@nturgut
Copy link
Contributor

@nturgut nturgut commented Jun 19, 2020

(1) change the version of simulator. luci bots only has 13.0
(2) skip failing tests
flutter/flutter#60040
flutter/flutter#60036
flutter/flutter#60033

Example run on infra: https://chromium-swarm.appspot.com/task?id=4cf78088e41b5310

Fixes: flutter/flutter#53690

@auto-assign auto-assign bot requested a review from GaryQian June 19, 2020 00:02
@nturgut nturgut marked this pull request as draft June 22, 2020 18:46
@nturgut nturgut marked this pull request as ready for review June 23, 2020 00:51
@auto-assign auto-assign bot requested a review from jason-simmons June 23, 2020 00:51
@nturgut nturgut requested review from harryterkelsen, mdebbar and yjbanov and removed request for GaryQian and jason-simmons June 23, 2020 00:51
/// is used for combining [shouldSkip] information with the `context`.
/// See: [_PointerEventContext] [_MouseEventContext] [_TouchEventContext].
/// TODO: https://github.com/flutter/flutter/issues/60033
class _BrowserUnitTestContext<T extends _BasicEventContext> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also considered using Tuple: https://pub.dev/packages/tuple

@mdebbar let me know if this looks good to you or should I changed to Tuple2.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is way more readable than Tuple2, because you can name and document the fields.

// @dart = 2.6
// This test failed on iOS Safari.
// TODO: https://github.com/flutter/flutter/issues/60040
@TestOn('chrome || firefox')
Copy link
Contributor

Choose a reason for hiding this comment

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

We should still run it in desktop Safari. The issue seems to be limited to the iOS Simulator: https://bugs.webkit.org/show_bug.cgi?id=191064.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Desktop safari is not running, there is a blocker on infra side.

Do you want to keep it for the local development?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. I tested it locally with Safari Desktop: All tests passed!.

I changed the test to only skip for iOS-Safari.

/// is used for combining [shouldSkip] information with the `context`.
/// See: [_PointerEventContext] [_MouseEventContext] [_TouchEventContext].
/// TODO: https://github.com/flutter/flutter/issues/60033
class _BrowserUnitTestContext<T extends _BasicEventContext> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is way more readable than Tuple2, because you can name and document the fields.

@nturgut nturgut requested a review from yjbanov June 23, 2020 18:08
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

LGTM if LGT @mdebbar

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 24, 2020
zhongwuzw pushed a commit to zhongwuzw/engine that referenced this pull request Jun 24, 2020
* change the version of simulator. luci bots only has 13.0

* skip failing pointer binding tests

* skip path_metrics (canvaskit) and a method from surface_test

* fix analyzer errors

* remove extra added touch event context test

* removing left overs for screenshot debugging

* apply reviewers suggestion for skipping tests
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[web] Add capability to run ios safari unit tests to felt

4 participants