Investigate and fix unit test passing on PRs but failing on push to main

mozilla-mobile / firefox-ios

Firefox for iOS

Mozilla Public License 2.0

12.1k stars 2.88k forks source link

Investigate and fix unit test passing on PRs but failing on push to main #19716

Open isabelrios opened 3 months ago

isabelrios commented 3 months ago

This is a strange situation in which unit tests are passing in PRs, all checks are green, but when the build runs on main after merge, they fail.

Please see issue with the test failures.

The failures were caused by this commit, which was upgrading both Glean and A-S dependencies.

Even with latest main with that commit included, failures were not reproducible locally.

In theory building for PRs and main is equal for the application. Steps and configuration in Bitrise is exactly the same.

Wondering if that would be the same for the dependencies and / or experiments

Main build: https://app.bitrise.io/build/15d0ee58-2972-45c4-983b-f49fc9d217fc?tab=log PR build: https://app.bitrise.io/build/f6cfcb54-b0ef-462c-bd89-246e82f36dca

┆Issue is synchronized with this Jira Task

isabelrios commented 3 months ago

Adding info that may be related to this issue shared by @linabutler in slack:

if a Remote Settings server URL isn't specified, nimbus falls back to creating a "null client" that doesn't return any experiments
the FxiOS Experiments wrapper reads the Remote Settings URL from a key in the Info.plist. i'm guessing this might not be set / used in the unit tests at all, and that's why it works locally?
it looks like bitrise.yml sets that Info.plist key to a $NIMBUS_URL variable, for beta, release, and nightly? where are you configuring that variable in bitrise?

i'm wondering if $NIMBUS_URL is set differently in PR checks vs. main (maybe the PR checks aren't building the beta / release / nightly variants?), and that's why the behavior was different?

if $NIMBUS_URL isn't set, then Experimenter.remoteSettingsURL will be nil, and nimbus will fall back to the "null client" and skip the path to fetch data from remote settings (which was the crashy branch). but if $NIMBUS_URL is set, it would've triggered the crash

isabelrios commented 3 months ago

I confirm that $NIMBUS_URL is not exposed to PRs in Bitrise's configuration, so if I understood correctly we skipped the path to fetch data from remote settings and so we did not see the crash... because the variable is set for checkins in main and builds that are built from there (beta, nightly) we saw the crash there.

isabelrios commented 3 months ago

The only way we could have detected this in PR is by exposing that url to be available to PRs, that does not mean it will be printed in the logs as far as I know... Would that be possible or we prefer to keep it as it is?