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.

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

Jump to bottom

Update Zulip tests and re-enable #441

Merged
merged 3 commits into from
Jan 21, 2025
Merged

Update Zulip tests and re-enable #441

merged 3 commits into from
Jan 21, 2025

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Jan 15, 2025 •
edited
Loading

This has the update to unblock autocomplete changes:
zulip/zulip-flutter#1190
flutter/flutter#143249

and with that, tests pass again on Flutter main.

Also install libsqlite3-dev on Linux, a package which is no longer
included in GitHub's new version of the "ubuntu-latest" images. The
lack of it causes nondeterministic failures when the runner happens
to use the new image:
#441 (comment)

The actual shared library file libsqlite3.so.0.8.6 -- in the package
libsqlite3-0 -- is still installed by default anyway. This package
adds the symlink at "libsqlite3.so", which is the filename some
libraries apparently look for.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@gnprice
Update Zulip tests and re-enable
c18abc5 
This has the update to unblock autocomplete changes:
  zulip/zulip-flutter#1190
  flutter/flutter#143249

and with that, tests pass again on Flutter main.
@gnprice
Copy link
Member Author

gnprice commented Jan 15, 2025

Tests pass in Zulip's own CI on Flutter main, after zulip/zulip-flutter#1190, so I expect they'll pass here too. We'll have confirmation on that soon.

@justinmc @Piinks I saw a mention at #437 (comment) that there was more than one upstream change these tests were blocking. I guess any others haven't yet landed, given that tests pass — what's the other PR (or PRs) involved? (I did just re-scan through Zulip's PR queue and my email, but didn't spot anything.) Happy to take updates as needed.

@gnprice gnprice mentioned this pull request Jan 15, 2025
Merged
8 tasks
@Piinks
Copy link
Contributor

Piinks commented Jan 15, 2025

There were a couple PRs that were seeing test flakes actually, @gnprice do you have a sense for how flaky some tests may be in this test suite?

@gnprice
Copy link
Member Author

gnprice commented Jan 15, 2025

We've had a flaky test or two in the past, but I'm not aware of any right now. If you can track down the affected test runs or PRs, I'd be glad to investigate.

@gnprice
Copy link
Member Author

gnprice commented Jan 15, 2025

(I've also now kicked off running our test suite a bunch of times in a loop, on my own machine — so if there is a flake currently present, hopefully that will find it.)

@Piinks
Copy link
Contributor

Piinks commented Jan 15, 2025

Other than this comment #437 (comment)
I also heard about it on https://github.com/flutter/flutter/pull/161091/checks which ran into

| ✅ /tmp/flutter_customer_testing.zulip.FCQOYP/tests/test/notifications/display_test.dart: NotificationOpenPayload parse: fails when host is not "notification"
|
| ::error::2120 tests passed, 5 failed, 8 skipped.
ERROR: One or more tests from zulip failed.
Tests finished in 76.33 seconds per iteration.

@gnprice
Copy link
Member Author

gnprice commented Jan 15, 2025

Thanks — looking into those.

@gnprice
Copy link
Member Author

gnprice commented Jan 15, 2025

I've investigated and I believe there are no flakes in the tests. There is an infra issue, which caused one and probably both of the failures mentioned above. I'll deal with that.

The failure at #437 (comment) comes from libsqlite3.so being missing on the system. It's currently nondeterministic whether that's present, because this repo's CI uses GitHub's ubuntu-latest image, and they're in the middle of a rollout to change what that means:

The failure on https://github.com/flutter/flutter/pull/161091/checks quoted above is hard to pin down because the checks have been re-run since then, and there doesn't seem to be a way to get more details from GitHub (though I spent some time digging for them). Do you have a more complete log than that snippet?

Lacking more info on that one, though: "5 failed" is exactly the number of tests that would fail for lack of libsqlite3.so, so with no sign of another issue it's probably that one.

Then to actively search out any flakes:

@gnprice
Install libsqlite3-dev for Zulip test on Linux
c219550 
This package is no longer included in GitHub's new version of the
"ubuntu-latest" images.  That causes nondeterministic failures when
the runner happens to use the new version:
  flutter#441 (comment)

The actual shared library file libsqlite3.so.0.8.6 -- in the package
libsqlite3-0 -- is still installed by default anyway.  This package
adds the symlink at "libsqlite3.so", which is the filename some
libraries apparently look for.
@gnprice
Simplify command
bd26c45
@gnprice
Copy link
Member Author

gnprice commented Jan 15, 2025

Fixed that infra issue by installing the package again. Also updated the PR description so it provides an accurate commit message for a squash-merge commit.

This was referenced Jan 17, 2025
Closed
Merged
Piinks
Piinks approved these changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Thanks for the thorough investigation! This LGTM!

@auto-submit auto-submit bot merged commit 8cf7a67 into flutter:main Jan 21, 2025
12 checks passed
@justinmc
Copy link

@gnprice Thanks for jumping on this right after the holidays!

@matanlurey
Copy link
Contributor

reason for revert: sudo apt-get install does not work on LUCI

@matanlurey matanlurey added the revert Label used to revert changes in a closed and merged pull request. label Jan 22, 2025
auto-submit bot pushed a commit that referenced this pull request Jan 22, 2025
Revert "Update Zulip tests and re-enable (#441)"
72074fb 
This reverts commit 8cf7a67.
@auto-submit auto-submit bot mentioned this pull request Jan 22, 2025
Merged
8 tasks
@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Jan 22, 2025
auto-submit bot added a commit that referenced this pull request Jan 22, 2025
@auto-submit
Reverts "Update Zulip tests and re-enable (#441)" (#444)
b4cc097 
Reverts: #441
Initiated by: matanlurey
Reason for reverting: sudo apt-get install does not work on LUCI
Original PR Author: gnprice

Reviewed By: {Piinks}

This change reverts the following previous change:
This has the update to unblock autocomplete changes:
  zulip/zulip-flutter#1190
  flutter/flutter#143249

and with that, tests pass again on Flutter main.

Also install libsqlite3-dev on Linux, a package which is no longer
included in GitHub's new version of the "ubuntu-latest" images.  The
lack of it causes nondeterministic failures when the runner happens
to use the new image:
  #441 (comment)

The actual shared library file libsqlite3.so.0.8.6 -- in the package
libsqlite3-0 -- is still installed by default anyway.  This package
adds the symlink at "libsqlite3.so", which is the filename some
libraries apparently look for.
@gnprice gnprice mentioned this pull request Jan 23, 2025
Closed
gnprice added a commit to gnprice/zulip-flutter that referenced this pull request Jan 23, 2025
@gnprice
ci: Add setup script for running in Flutter "customer testing"
f5d2a45 
Earlier I'd tried adding this "apt install" command directly in the
test registry, as a "setup" line in the "zulip.test" file:
  flutter/tests#441

But the Flutter "customer testing" suite gets run in two different
environments; and it turns out that not only does the LUCI environment
not need this step (the Zulip tests there were working fine right up
until they were disabled last week due to the failures in GitHub
Actions), but it also doesn't permit it: there's no access to `sudo`:
  https://discord.com/channels/608014603317936148/1290464157765865552/1331449169830871122
  https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20customer_testing/22301/overview

Error output (reformatted):

    Processing ./../../bin/cache/pkg/tests/registry/zulip.test...
    >> sudo apt install -y libsqlite3-dev
    | sudo: a terminal is required to read the password;
      either use the -S option to read from standard input
      or configure an askpass helper
    ERROR: Setup command failed: sudo apt install -y libsqlite3-dev

So we need this bit of conditional logic too.  That makes this a
little more complex than fits in a "setup" line in the test registry.
So instead let's make this script which we can invoke from there.
@gnprice gnprice mentioned this pull request Jan 23, 2025
Merged
@gnprice gnprice deleted the pr-zulip-update branch January 23, 2025 22:45
chrisbobbe pushed a commit to zulip/zulip-flutter that referenced this pull request Jan 23, 2025
@gnprice @chrisbobbe
ci: Add setup script for running in Flutter "customer testing"
ae7939a 
Earlier I'd tried adding this "apt install" command directly in the
test registry, as a "setup" line in the "zulip.test" file:
  flutter/tests#441

But the Flutter "customer testing" suite gets run in two different
environments; and it turns out that not only does the LUCI environment
not need this step (the Zulip tests there were working fine right up
until they were disabled last week due to the failures in GitHub
Actions), but it also doesn't permit it: there's no access to `sudo`:
  https://discord.com/channels/608014603317936148/1290464157765865552/1331449169830871122
  https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20customer_testing/22301/overview

Error output (reformatted):

    Processing ./../../bin/cache/pkg/tests/registry/zulip.test...
    >> sudo apt install -y libsqlite3-dev
    | sudo: a terminal is required to read the password;
      either use the -S option to read from standard input
      or configure an askpass helper
    ERROR: Setup command failed: sudo apt install -y libsqlite3-dev

So we need this bit of conditional logic too.  That makes this a
little more complex than fits in a "setup" line in the test registry.
So instead let's make this script which we can invoke from there.
gnprice added a commit to gnprice/flutter_customer_testing that referenced this pull request Jan 23, 2025
@gnprice
Re-land "Update Zulip tests and re-enable (flutter#441)"
99c41db 
This re-lands 8cf7a67.

Difference from previous version: This version has the new setup step
do its work only where `sudo` is available (in GitHub Actions), and
successfully do nothing where it's unavailable (in LUCI).  The step
isn't needed in LUCI.

The logic lives in a setup script added today to the Zulip tree:
  zulip/zulip-flutter#1300
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Piinks Piinks Piinks approved these changes

Assignees
No one assigned
Labels
autosubmit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gnprice @Piinks @justinmc @matanlurey