[RunAllTests] Remove caching for Bazel CI workflows #2884

BenHenning · 2021-03-11T07:44:42Z

Bazel caching is currently not cost effective: it only caches 25-30% of actions, and we're not seeing worthwhile performance benefits from it. This PR makes caching configurable for both unit tests and binary builds, and disables it for CI workflows. #1861 is tracking improving this over time, including upstream patching Bazel itself to fix some of the caching issues. We may revisit
reenabling caching at that time.

Runs verifying that caching is now disabled:

Builds

Unit tests

Remove caching from Bazel tests.

Remove caching for Bazel builds.

BenHenning · 2021-03-11T07:45:47Z

@seanlip @rt4914 @anandwana001 assigning this broadly to try and get the PR in sooner since it's a priority. Please feel free to let it get auto-merged if any of you approve it (no need for more than one approval here, I think). Please reach out if you have any questions or concerns.

seanlip

Thanks, LGTM!

seanlip · 2021-03-11T07:46:29Z

(or not?)

EDIT: Ah, ignore that. I was confused about why the PR got closed, which is explained in the next comment.

BenHenning · 2021-03-11T07:46:40Z

Restarting CI to force Bazel tests to run to verify that they are actually not using caching anymore.

.github/workflows/build_tests.yml

.github/workflows/unit_tests.yml

Fix environment variable conditions.

BenHenning · 2021-03-11T07:55:40Z

Thanks for the super fast review @seanlip!

## Explanation Fixes #5012 Removes references and code corresponding to KitKat (SDK version 19) since the app has been set to a minimum version of Lollipop (SDK 21) since #3910 (roughly--technically KitKat was still supported but we no longer shipped a KitKat version). Because of this, the changes in this PR are largely a no-op from a build perspective. More broadly, this change is motivated by a desire to decrease CI time (which was already reduce considerably in the recent merge of #5629) by removing extraneous app builds being done in CI: - ``//:oppia`` since we should really only use ``//:oppia_dev`` moving forward. - Dev and alpha KitKat builds (the latter of which takes a long time since only the alpha job was building 2 proguarded builds of the app and thus taking much more time than the beta and GA build jobs). Separately, ``build_tests.yml`` and ``unit_tests.yml`` were updated to no longer support caching since we disabled this behavior a while back (#2884) and are unlikely to reintroduce it due to high storage costs. Some other noteworthy changes: - The optional providing of ``Retrofit`` and Retrofit services has been reverted since it's no longer necessary (and corresponding tests have been removed since there's no longer any condition to verify). - Code that was gated to run below Lollipop has been removed since it can't execute except on KitKat devices. - Support for a main dex target list has been removed as we're unlikely to need it with native multidex support (which wasn't an option for KitKat builds), though we may choose to reintroduce it to speed up cold app starts since it _can potentially_ help improve time-to-splash app startup performance. - Updated manifest min SDK values to avoid potential developer confusion on the min SDK version supported (though, strictly speaking, this doesn't technically matter with the way the app builds). - This updates the default min version supported for OS deprecation. No additional work is needed to actually activate the deprecation notice on the KitKat version of the app since we no longer deploy it, and OS deprecation wasn't added to the last version that was deployed. - Three file content pattern failure messages were updated to no longer reference KitKat (though the value of these checks mostly still seems realized, so I think it's fine to keep them in). - Some additional licenses were added due to tooling around the dev AAB (even though these aren't dependencies that ship with the app). I think it's fine including extra licenses in this case. - The protobuf license was corrected to BSD 3-Clause as declared for the dependency and in the GitHub repository for the dependencies. - This bumps version codes due to removing two flavors (for KitKat dev and KitKat alpha). - ``NetworkModuleTest`` has had its tests redesigned and better filled in in order to pass the code coverage minimum requirement (since old tests were removed). Note that it's not quite perfect in what it's testing, but it is at least verifying the complete configuration of the ``Retrofit`` instance, and the singleton properties of the two services currently supported (verifying that the services are set up would require a lot more testing that seems outside the direct scope of this test, so it seems okay to ignore here). - As part of the previous item's work, ``NetworkLoggingInterceptor`` had an issue fixed where it could cause an OkHttp exception to be thrown when trying to process logs (due to reading the response body twice). Reverting this change will now cause breakage failures in ``NetworkModuleTest``. - ``NetworkModule``'s initialization order was changed for slightly better network request logging (see the new comment in that file for more context). Note that there are some workflow job removals and renames in this PR. Any required CI checks will be correspondingly updated once this PR is approved and ready to merge (to avoid blocking any other in-flight PRs). ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only This shouldn't have an side effects on non-KitKat UIs, and KitKat support itself is being removed in this PR (so there's nothing to demonstrate).

## Explanation Fixes oppia#5012 Removes references and code corresponding to KitKat (SDK version 19) since the app has been set to a minimum version of Lollipop (SDK 21) since oppia#3910 (roughly--technically KitKat was still supported but we no longer shipped a KitKat version). Because of this, the changes in this PR are largely a no-op from a build perspective. More broadly, this change is motivated by a desire to decrease CI time (which was already reduce considerably in the recent merge of oppia#5629) by removing extraneous app builds being done in CI: - ``//:oppia`` since we should really only use ``//:oppia_dev`` moving forward. - Dev and alpha KitKat builds (the latter of which takes a long time since only the alpha job was building 2 proguarded builds of the app and thus taking much more time than the beta and GA build jobs). Separately, ``build_tests.yml`` and ``unit_tests.yml`` were updated to no longer support caching since we disabled this behavior a while back (oppia#2884) and are unlikely to reintroduce it due to high storage costs. Some other noteworthy changes: - The optional providing of ``Retrofit`` and Retrofit services has been reverted since it's no longer necessary (and corresponding tests have been removed since there's no longer any condition to verify). - Code that was gated to run below Lollipop has been removed since it can't execute except on KitKat devices. - Support for a main dex target list has been removed as we're unlikely to need it with native multidex support (which wasn't an option for KitKat builds), though we may choose to reintroduce it to speed up cold app starts since it _can potentially_ help improve time-to-splash app startup performance. - Updated manifest min SDK values to avoid potential developer confusion on the min SDK version supported (though, strictly speaking, this doesn't technically matter with the way the app builds). - This updates the default min version supported for OS deprecation. No additional work is needed to actually activate the deprecation notice on the KitKat version of the app since we no longer deploy it, and OS deprecation wasn't added to the last version that was deployed. - Three file content pattern failure messages were updated to no longer reference KitKat (though the value of these checks mostly still seems realized, so I think it's fine to keep them in). - Some additional licenses were added due to tooling around the dev AAB (even though these aren't dependencies that ship with the app). I think it's fine including extra licenses in this case. - The protobuf license was corrected to BSD 3-Clause as declared for the dependency and in the GitHub repository for the dependencies. - This bumps version codes due to removing two flavors (for KitKat dev and KitKat alpha). - ``NetworkModuleTest`` has had its tests redesigned and better filled in in order to pass the code coverage minimum requirement (since old tests were removed). Note that it's not quite perfect in what it's testing, but it is at least verifying the complete configuration of the ``Retrofit`` instance, and the singleton properties of the two services currently supported (verifying that the services are set up would require a lot more testing that seems outside the direct scope of this test, so it seems okay to ignore here). - As part of the previous item's work, ``NetworkLoggingInterceptor`` had an issue fixed where it could cause an OkHttp exception to be thrown when trying to process logs (due to reading the response body twice). Reverting this change will now cause breakage failures in ``NetworkModuleTest``. - ``NetworkModule``'s initialization order was changed for slightly better network request logging (see the new comment in that file for more context). Note that there are some workflow job removals and renames in this PR. Any required CI checks will be correspondingly updated once this PR is approved and ready to merge (to avoid blocking any other in-flight PRs). ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only This shouldn't have an side effects on non-KitKat UIs, and KitKat support itself is being removed in this PR (so there's nothing to demonstrate).

BenHenning added 2 commits March 10, 2021 23:41

Update unit_tests.yml

8301cf9

Remove caching from Bazel tests.

Update build_tests.yml

1348665

Remove caching for Bazel builds.

BenHenning requested review from anandwana001, rt4914 and seanlip March 11, 2021 07:45

BenHenning assigned rt4914, seanlip and anandwana001 Mar 11, 2021

seanlip approved these changes Mar 11, 2021

View reviewed changes

BenHenning changed the title ~~Remove caching for Bazel CI workflows~~ [RunAllTests] Remove caching for Bazel CI workflows Mar 11, 2021

BenHenning closed this Mar 11, 2021

seanlip removed their assignment Mar 11, 2021

BenHenning reopened this Mar 11, 2021