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

Use the Pixel 7/API 33 device in the Firebase test lab script #50721

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

jason-simmons
Copy link
Member

This test is currently failing consistently when running with Pixel 8/API 34

This test is currently failing consistently when running with Pixel 8/API 34
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Copy link
Member

@gmackall gmackall left a comment

Choose a reason for hiding this comment

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

LGTM to get the tree green, though I would be curious if "husky" (pixel 8 pro) passes (ideally we wouldn't regress off of api34)

@jason-simmons jason-simmons added warning: land on red to fix tree breakage Override tree-status signal (land even with closed tree), combine with the autosubmit label. autosubmit Merge PR when tree becomes green via auto submit App labels Feb 16, 2024
@auto-submit auto-submit bot merged commit 5fd5ccf into flutter:main Feb 16, 2024
30 of 31 checks passed
@jason-simmons
Copy link
Member Author

Tried a run with model=husky,version=34. It also resulted in an infrastructure error on FTL:
https://console.firebase.google.com/project/flutter-infra-staging/testlab/histories/bh.6b85e5d132218e8f/matrices/8478154452546318566

@gmackall
Copy link
Member

I was going to comment that it is interesting that they are still working on the flutter/flutter dashboard
https://flutter-dashboard.appspot.com/#/build
But they are just failing? But not blocking the tree...
https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20firebase_android_embedding_v2_smoke_test/17576/overview

┌──────────────┬────────────────────────┬────────────────────────┐
│   OUTCOME    │    TEST_AXIS_VALUE     │      TEST_DETAILS      │
├──────────────┼────────────────────────┼────────────────────────┤
│ Passed       │ griffin-24-en-portrait │ --                     │
│ Passed       │ redfin-30-en-portrait  │ --                     │
│ Inconclusive │ shiba-34-en-portrait   │ Infrastructure failure │
└──────────────┴────────────────────────┴────────────────────────┘

@reidbaker
Copy link
Contributor

@jason-simmons does this indicate a real bug on api 34 or something that is a test only failure?

@reidbaker
Copy link
Contributor

Unless there is evidence that this is a test only failure I dont think changing what we test to get the tree green is the right decision.

@zanderso
Copy link
Member

I filed flutter/flutter#143590 for the framework CI non-failure.

@zanderso
Copy link
Member

@jason-simmons does this indicate a real bug on api 34 or something that is a test only failure?

The only thing we know at this time is that Firebase test lab is reporting an infrastructure failure for that configuration.

@zanderso
Copy link
Member

To be clear though, I don't know what you mean by a "test only failure".

@jason-simmons
Copy link
Member Author

Firebase Test Lab is reporting the test status as "inconclusive", which IIUC typically means an infrastructure issue within FTL.

I did not notice anything in the FTL logs that explained the reason for the failure. Apps built with the ToT engine seem to be behaving as expected on my local Pixel 6a running API 34.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 16, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 16, 2024
…143593)

flutter/engine@dd530f1...5fd5ccf

2024-02-16 jason-simmons@users.noreply.github.com Use the Pixel 7/API 33 device in the Firebase test lab script (flutter/engine#50721)
2024-02-16 leroux_bruno@yahoo.fr [Web] Ignore invalid keyboard events related to autofill (flutter/engine#50590)
2024-02-16 jason-simmons@users.noreply.github.com Add the web package to the set of Dart third_party packages in DEPS (flutter/engine#50714)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jsimmons@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@reidbaker
Copy link
Contributor

To be clear though, I don't know what you mean by a "test only failure".

I was trying to distinguish between and issue that was present in our code and an issue that was present in our testing code.

Gray and I paired on 2 prs to try to help prove that this is an FTL hardware issue
flutter/flutter#143596 and flutter/flutter#143595
Both show that this is likely something on their end.

Additionally flutter/flutter#143590 (comment) tells us that the engine and framework respond differently to FTL reporting infra issues. There is a error code we can use to ignore failures if we want.

jason-simmons added a commit to jason-simmons/flutter_engine that referenced this pull request Feb 16, 2024
…handle FTL infrastucture errors in the script

This test had been temporarily moved to an API 33 device as a workaround for an FTL infrastructure issue (see flutter#50721)

This PR resumes use of a Pixel 8/API 34 device for the FTL tests.  It also retries the test if FTL returns an error code that is known to represent an FTL infrastructure error.  If the retries fail, then the test script will not block the engine tree if all failures were caused by infrastructure.

This is similar to what the CI recipes are doing in https://flutter.googlesource.com/recipes/+/a181878fde742dacd94afca04c5a4db1b3c30b91
jason-simmons added a commit to jason-simmons/flutter_engine that referenced this pull request Feb 16, 2024
…handle FTL infrastructure errors in the script

This test had been temporarily moved to an API 33 device as a workaround for an FTL infrastructure issue (see flutter#50721)

This PR resumes use of a Pixel 8/API 34 device for the FTL tests.  It also retries the test if FTL returns an error code that is known to represent an FTL infrastructure error.  If the retries fail, then the test script will not block the engine tree if all failures were caused by infrastructure.

This is similar to what the CI recipes are doing in https://flutter.googlesource.com/recipes/+/a181878fde742dacd94afca04c5a4db1b3c30b91
auto-submit bot pushed a commit that referenced this pull request Feb 17, 2024
…handle FTL infrastructure errors in the script (#50735)

This test had been temporarily moved to an API 33 device as a workaround for an FTL infrastructure issue (see #50721)

This PR resumes use of a Pixel 8/API 34 device for the FTL tests.  It also retries the test if FTL returns an error code that is known to represent an FTL infrastructure error.  If the retries fail, then the test script will not block the engine tree if all failures were caused by infrastructure.

This is similar to what the CI recipes are doing in https://flutter.googlesource.com/recipes/+/a181878fde742dacd94afca04c5a4db1b3c30b91
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App warning: land on red to fix tree breakage Override tree-status signal (land even with closed tree), combine with the autosubmit label.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants