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

Add zulip.test #368

Merged
merged 3 commits into from
May 23, 2024
Merged

Add zulip.test #368

merged 3 commits into from
May 23, 2024

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented May 22, 2024

Zulip is an open-source team chat app, with a new Flutter-based mobile client now in beta. This is that client's test suite.

I believe these will be the only tests currently in this registry for an app, rather than a library. That should naturally give them a different mix of use cases and types of tests.

Concretely, we've seen a handful of breaking changes over the past year that weren't caught by any of Flutter's existing test suites. These were in text hit-testing:
zulip/zulip-flutter@ba7a2bf
flutter/flutter#140621

and Material menu behavior:
zulip/zulip-flutter@38ed6c8
flutter/flutter#130536

and SlottedContainerRenderObjectMixin gaining a type parameter:
zulip/zulip-flutter@2f0f469
flutter/flutter#126108

I'm not complaining, to be clear, and none of these were particularly onerous for us to adapt to. By registering these tests, I'm hoping to provide feedback on future such breakages at a point where it's actionable.

Omitted here are several tests that re-generate generated files and check they match what's in the tree. Those are pretty slow, and I think they're pretty insensitive to changes in the Flutter tree anyway; rather they depend on pigeon, json_serializable, build_runner, and drift_dev.

Zulip is an open-source team chat app, with a new Flutter-based
mobile client now in beta.  This is that client's test suite.

I believe these will be the only tests currently in this registry
for an app, rather than a library.  That should naturally give them
a different mix of use cases and types of tests.

Concretely, we've seen a handful of breaking changes over the past
year that weren't caught by any of Flutter's existing test suites.
These were in text hit-testing:
  zulip/zulip-flutter@ba7a2bf
  flutter/flutter#140621

and Material menu behavior:
  zulip/zulip-flutter@38ed6c8
  flutter/flutter#130536

and SlottedContainerRenderObjectMixin gaining a type parameter:
  zulip/zulip-flutter@2f0f469
  flutter/flutter#126108

I'm not complaining, to be clear, and none of these were
particularly onerous for us to adapt to.  By registering these
tests, I'm hoping to provide feedback on future such breakages
at a point where it's actionable.

Omitted here are several tests that re-generate generated files
and check they match what's in the tree.  Those are pretty slow,
and I think they're pretty insensitive to changes in the Flutter
tree anyway; rather they depend on pigeon, json_serializable,
build_runner, and drift_dev.
@gnprice
Copy link
Member Author

gnprice commented May 22, 2024

These do take longer than the "100 times in 15 minutes" guideline: on my machine, 100 iterations takes about 35 minutes. I don't think they're doing anything especially slow; I think it's just that we have a lot of tests. But naturally please feel free to take a look and make your own judgement.

@gnprice
Copy link
Member Author

gnprice commented May 22, 2024

I think this may be the first time our tests have ever been run on Windows. 😅 There's one failure, which I've just sent a fix for:

I'll update this PR once that's merged.

@gnprice
Copy link
Member Author

gnprice commented May 22, 2024

There's also a failure in flutter_devtools:
https://github.com/flutter/tests/actions/runs/9197725826/job/25298837719?pr=368#step:3:3581
Possibly-related issues are:

It looks like there's active work on flutter/flutter#148348, so I'll assume this is a flake and let that issue track fixing its cause.

@gnprice
Copy link
Member Author

gnprice commented May 22, 2024

Hmm, still failing on Windows (and still passing on Linux and macOS). But I'm puzzled at the remaining failure:

2024-05-22T22:28:28.0921182Z Running tests...
2024-05-22T22:28:28.0921690Z Round 1 of 3.
2024-05-22T22:28:28.0922434Z >> flutter analyze --no-fatal-infos
2024-05-22T22:28:50.0171317Z | Analyzing tests...
2024-05-22T22:28:50.0187035Z | No issues found! (ran in 20.7s)
2024-05-22T22:28:50.0417986Z >> flutter test
2024-05-22T22:28:51.6364767Z |
2024-05-22T22:28:59.3159233Z | ✅ C:/Users/RUNNER~1/AppData/Local/Temp/flutter_customer_testing.zulip.504902a4/tests/test/api/backoff_test.dart: BackoffMachine timeouts are random from zero to 100ms, 200ms, 400ms, ...
[…]
2024-05-22T22:29:49.9791559Z | ✅ C:/Users/RUNNER~1/AppData/Local/Temp/flutter_customer_testing.zulip.504902a4/tests/test/widgets/unread_count_badge_test.dart: UnreadCountBadge background stream color
2024-05-22T22:29:50.0008684Z |
2024-05-22T22:29:50.0016766Z | 🎉 1141 tests passed, 9 skipped.
2024-05-22T22:29:50.0915435Z ERROR: One or more tests from zulip failed.
2024-05-22T22:29:50.0915881Z Round 2 of 3.

So the output of both flutter analyze and flutter test appear happy — the latter has 1141 tests passed, a few skipped, and none failed. But nevertheless "One or more tests from zulip failed."

@gnprice
Copy link
Member Author

gnprice commented May 22, 2024

Looking around at other suites in the registry:

Several don't run on Windows: flutter_svg, flutter_devtools, flutter_cocoon. (And macos_ui is disabled entirely.)

Among those that do run, dart_rfb has exactly the same two test command lines, and similar output:

2024-05-22T22:27:07.9926354Z Running tests...
2024-05-22T22:27:07.9926798Z Round 1 of 10.
2024-05-22T22:27:07.9927444Z >> flutter analyze --no-fatal-infos
2024-05-22T22:27:12.8110985Z | Analyzing tests...
2024-05-22T22:27:12.8130007Z | No issues found! (ran in 3.9s)
2024-05-22T22:27:12.8328853Z >> flutter test
2024-05-22T22:27:13.9616495Z |
2024-05-22T22:27:15.4058599Z | ❎ C:/Users/RUNNER~1/AppData/Local/Temp/flutter_customer_testing.dart_rfb.6f4e275d/tests/test/client_test.dart: Connect to VNC running on localhost (skipped)
[…]
2024-05-22T22:27:19.0653626Z | ✅ C:/Users/RUNNER~1/AppData/Local/Temp/flutter_customer_testing.dart_rfb.6f4e275d/tests/test/protocol/set_pixel_format_message_test.dart: Set pixel format message to bytes works
2024-05-22T22:27:19.0740089Z |
2024-05-22T22:27:19.0746919Z | 🎉 19 tests passed, 1 skipped.
2024-05-22T22:27:19.1148432Z Round 2 of 10.

but there's no "ERROR: One or more tests from zulip failed." line, and the suite passes.

In particular, that one has a skipped test too, so it's not like somehow having a skip is causing the failure.


I don't really have any other ideas. I looked at dev/customer_testing/lib/runner.dart in the Flutter tree, where that error message is coming from, and I don't spot anything potentially amiss — it seems like the error has to mean that the flutter test command produced a nonzero Process.exitCode. And I don't know why flutter test would have done that when it reported no failures in its output.


So if anyone has ideas for why flutter test seems to be producing that mismatch of output and error code on Windows, I'm curious.

Failing that, my inclination would be to use test.posix= and skip running these on Windows. I think it's unlikely that any changes in Flutter that break the Zulip tests would do so on Windows without equally doing so on Linux and macOS; we don't normally ever run on Windows, and don't have any specific logic for it.

@gnprice
Copy link
Member Author

gnprice commented May 23, 2024

OK, skipped Windows. All tests are now passing.

Copy link
Contributor

@Hixie Hixie left a comment

Choose a reason for hiding this comment

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

LGTM

@gnprice gnprice merged commit 54cc35f into flutter:main May 23, 2024
12 checks passed
@gnprice gnprice deleted the pr-zulip branch May 23, 2024 18:03
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