FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Reduce number of RewardedAd loads on iOS in CI by DellaBitta · Pull Request #1180 · firebase/firebase-cpp-sdk · GitHub

/ firebase-cpp-sdk Public

Reduce number of RewardedAd loads on iOS in CI - #1180

Merged
DellaBitta merged 6 commits into
mainfrom
feature/gma_stress_test_changes
Jan 3, 2023
ConversationCommits (6)ChecksFiles changed
Merged

Reduce number of RewardedAd loads on iOS in CI#1180
DellaBitta merged 6 commits into
mainfrom
feature/gma_stress_test_changes

Conversation

DellaBitta commented Jan 3, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Provide details of the change, and generalize the change in the PR title above.

The GMA backend doesn't whitelist iOS devices running in CI which means number of ads that can be served to iOS in CI is restricted. This is true even when using the prescribed Demo Ad Unit Id.

This PR reduces the number of ads we load on iOS in an attempt to minimize the chance of encountering NoFillErrors and push our CI to green.

Notes:

  • Test a subset of RewardedAds behavior on iOS in GHA CI.
    • We no longer test successful RewardedAd loads.
    • Rewarded Ads stress tests are disabled on iOS.
  • Added a NoFill error as an acceptable result on stress tests for other platforms. The tests were meant to stress the SDK though ad object creation and destruction. The result from the service is secondary, and NoFill would be a valid result.
    • These tests are not enabled for iOs, though, as the service often goes into backoff mode on iOS. Backoff mode is represented by an obscure Internal Error code which could mean many different things.
  • All of the tests can still be executed locally on a device simply by building the test app. We should manually run the iOS testapp whenever we update the GMA dependencies.
  • Remove Flaky block retries around tests. The "flaky" behavior stemmed from NoFill errors.
    • Retrying on a NoFill error will continue to return NoFill errors since the the pool of ads to serve has been exhausted.
    • Retrying will make the situation worse:
      • The test will still eventually fail, it will just take longer to do so.
      • Repeated NoFill errors can push the service into back-off mode which may take longer to recover from.
    • It's better to wait a few minutes and re-run the test.

Testing

Describe how you've tested these changes. Link any manually triggered Integration tests or CPP binary SDK Packaging Github Action workflows, if applicable.

Integration Test CI

Type of Change

Place an x the applicable box:

  • Bug fix. Add the issue # below if applicable.
  • New feature. A non-breaking change which adds functionality.
  • Other, such as a build process or documentation change.

DellaBitta added the skip-release-notes Skip release notes check label Jan 3, 2023
DellaBitta requested a review from chkuang-g January 3, 2023 14:38
DellaBitta marked this pull request as ready for review January 3, 2023 14:39
DellaBitta commented Jan 3, 2023
View reviewed changes
Comment thread gma/integration_test/src/integration_test.cc
// Loading Ads has been deemed flaky as the AdMob Service has a chance to
// return NoFill for valid ad requests if there aren't any ads to serve.
FLAKY_TEST_SECTION_BEGIN();

DellaBitta Jan 3, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The flakiness stemmed from NoFill error when the service runs out of Ads to serve. Being in a state of NoFill often takes a lot longer to recover from than our Flaky Test retry timeout, and so the flaky test block causes more problems than it solves by 1: retesting and getting the same result, and 2: pushing the service into backoff mode if too many NoFill errors are generated.

DellaBitta merged commit e61d6bb into main Jan 3, 2023
DellaBitta deleted the feature/gma_stress_test_changes branch January 3, 2023 23:17
github-actions Bot added the tests: in-progress This PR's integration tests are in progress. label Jan 3, 2023

github-actions Bot commented Jan 3, 2023 •
edited
Loading

Copy link
Copy Markdown

❌  Integration test FAILED

Requested by @DellaBitta on commit e61d6bb
Last updated: Wed Jan 4 12:18 PST 2023
View integration test log & download artifacts

Failures Configs
missing_log [BUILD] [ERROR] [tvOS] [macos]

Add flaky tests to go/fpl-cpp-flake-tracker

github-actions Bot added the tests: failed This PR's integration tests failed. label Jan 3, 2023
firebase-workflow-trigger Bot removed the tests: in-progress This PR's integration tests are in progress. label Jan 4, 2023
tom-andersen pushed a commit that referenced this pull request Jan 30, 2023
Reduce number of RewardedAd loads on iOS in CI (#1180)
40e7025 
The GMA backend doesn't whitelist iOS devices running in CI which means number of ads that can be served to iOS in CI is restricted. This is true even when using the prescribed Demo Ad Unit Id.

This PR reduces the number of ads we load on iOS in an attempt to minimize the chance of encountering NoFillErrors and push our CI to green.
firebase locked and limited conversation to collaborators Feb 3, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
chkuang-g chkuang-g approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip-release-notes Skip release notes check tests: failed This PR's integration tests failed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL