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

[RunAllTests] Fix #5303, #5304, #5305, #5306, #5309, part of #5307, part of #5308: Fix a variety of dev platform-specific issues #5138

Merged
merged 30 commits into from
Feb 14, 2024

Conversation

BenHenning
Copy link
Member

@BenHenning BenHenning commented Aug 22, 2023

Explanation

Fixes #5303
Fixes #5304
Fixes #5305
Fixes #5306
Fixes #5309
Fixes part of #5307
Fixes part of #5308

This PR fixes a variety of issues that were discovered while trying to begin development on #4929 with a different workstation than I usually build on.

Some of the issues were outright problems at the outset, but others were discovered when trying to fix those issues (e.g. during test utility upgrades).

Specific issues that were found and fixed:

  • [BUG]: AssertionHelpers.assertThrows() doesn't handle AssertionErrors correctly #5303: AssertionHelpers.assertThrows didn't handle cases when trying to catch an AssertionError itself (such in the case of TestGitRepositoryTest). This could result in false positives, and it has since been fixed. Plus, the changes to AssertionHelpers also removes the dependency on Kotlin reflection.
  • [BUG]: LoggingIdentifierController has potential non-determinism in test environments #5304: LoggingIdentifierController could have ID generation inconsistencies between environments due to how its Random instance was initialized. This could also result in a test order issue if a subset of tests were run, or run in a different order. The controller has been updated to have better robustness against the order in which IDs are generated which fixes the test and environment determinism issues.

    The fix was essentially generating ID-specific seeds for ID-specific randoms that all originate from the application seed, and are always generated in the same order deterministically at the creation time of the controller. This fixes the IDs being generated in different orders leading to inconsistencies. Note that this is why ID changes are seen in a variety of tests (since there are now new seeds being used for generating these IDs as a by-product of the robustness improvements).
  • [BUG]: model/src/main/proto/BUILD.bazel has build issues #5305: model/src/main/proto/BUILD.bazel needed a fix due to a build warning (that only occurs when trying to build all targets).
  • [BUG]: TestGitRepository can have environment-specific behavior #5306: TestGitRepository was made more robust by better managing and setting the configured user & email used in its Git commands. The utility now manages a locally set configuration for each rather than relying on the global configuration (which may not be present on all systems, hence the original issue). Some other improvements in error detection were also added. Note that this likely wasn't discovered in CI because it seems that GitHub CI does set up a Git user & email automatically, as do most people who work on the project.
  • [BUG]: ActivityScenarioRule causes issues with platform parameter initialization #5307 & [BUG]: ActivityTestRule is deprecated #5308: ActivityTestRule is now deprecated in favor of ActivityScenarioRule since it can interact with activities outside their lifecycle. However, the scenario rule is also not good to use anymore since it can result in partial dependencies being initialized before test-only platform parameter states can be overridden. This PR introduces a new pattern to prevent using either of these rules, and the mentioned issues can be used to track the remaining tests that need to be cleaned up following this PR.
  • [BUG]: ProfileAndDeviceIdFragmentTest has order & machine-dependent behavior #5309: ProfileAndDeviceIdFragmentTest is environment and order-dependent due to out-of-order platform parameter initialization. This issue ultimately arose from test parameters being initialized after they're needed in the launched UI. It is necessary when there are environment differences (e.g. local vs. CI) or when running certain tests individually. This was fixed by removing the usage of ActivityScenarioRule and instead managing the fragment's test activity lifecycle directly.

    Separately, the test performs proto comparisons that can be machine-dependent. It seemed the test was suffering from some proto encoding inconsistencies that seem to occur between some development machines vs. on CI. The fix improves the test's robustness by extracting the raw encoded string, verifying that the other outputs in the intent message correctly correspond to that string, and that the string (as a parsed proto) contains the correct values. As a result, the test no longer depends on a hardcoded encoding value to be present for verification. This does result in a slightly lengthier test with more logic, but should be more stable in the long-term.

Some other miscellaneous notes:

  • testCreateLearnerId_verifyCreatesCorrectRandomValue was removed from LoggingIdentifierControllerTest because it wasn't actually providing useful testing value. The application seed is not itself part of the class's public API. Instead, the ID generation methods should be ultimately verified.
  • ComputeAffectedTestsTest had a shard increase to 24 and BazelClientTest has now been sharded to 4 (ditto for GenerateMavenDependenciesListTest and MavenDependenciesListCheckTest). Both were done to help distribute the more expensive tests in each suite across multiple runners to try and reduce the long-tail of script test runs. More optimization will probably be useful in the future. Note also that some of these tests are much slower on certain systems (the one I've been working on, for instance), hence the need for these adjustments.
  • testEnsureNextResultIsSuccess_failureThenSuccess_notified_throwsException was removed from DataProviderTestMonitorTest since it seems that it wasn't actually written correctly (and the issue wasn't discovered until assertThrows was fixed). Per the existing test coverage, it doesn't seem necessary to try and fix the test so it was instead removed.
  • ProfileAndDeviceIdFragmentTest changes led to a new EventLogSubject.hasNoProfileId being added.
  • There are a few miscellaneous script infrastructure changes that were pulled into this PR in preparation for downstream work. Specifically:
    • ComputeAffectedTests now allows for its command executor to be used for Bazel operations (which provides its tests with a bit more execution flexibility). Its tests were also updated to use a longer timeout for commands to try and improve test stability. BazelClientTest was similarly updated.
    • TestBazelWorkspace was updated to:
      • Have better workspace initialization robustness (since the workspace can actually be generated at several different points, not singularly with a call to initEmptyWorkspace).
      • Add a .bazelrc file to disable bzlmod (since it causes network stability issues in some environments when using newer Bazel versions even if the project isn't upgraded, and we don't use bzlmod yet, anyway).
      • Add a .bazelversion file to ensure that tests are using the same Bazel version as the project build.
  • AuthenticationControllerTest was largely updated since the changes to assertThrows behavior revealed that the failure test wasn't actually correctly verifying an exception was being passed (since it isn't actually being thrown; the test was a false positive possibly because it was using Throwable, but I didn't dig into exactly why it was passing before). The changes ensure that the callbacks are actually verified rather than the user result (since there's a separate test to do that), as well as the exception's failure message.

Essential Checklist

  • 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: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

N/A since all of the changes in this PR are infrastructural or only affect tests.

These issues were found after I started using a new development
environment.
ProfileAndDeviceIdFragmentTest had been updated to use a newer fragment
initialization pattern, but that's no longer needed and seems to be
causing what appears to be timing discrepancies between local dev and
CI.
@BenHenning BenHenning changed the title Fix a variety of dev platform-specific issues [RunAllTests] Fix a variety of dev platform-specific issues Aug 28, 2023
@BenHenning BenHenning closed this Aug 28, 2023
@BenHenning BenHenning reopened this Aug 28, 2023
@oppiabot
Copy link

oppiabot bot commented Sep 4, 2023

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added stale Corresponds to items that haven't seen a recent update and may be automatically closed. and removed stale Corresponds to items that haven't seen a recent update and may be automatically closed. labels Sep 4, 2023
The issue ultimately arose from test parameters being initialized after
they're needed in the launched UI. This type of change was tried earlier
in the branch, but reverted since it didn't seem necessary. It is,
however, necessary when there are environment differences (e.g. local
vs. CI) or when running certain tests individually.

Due to the difficulty in finding this issue, ActivityScenarioRule has
been added as a prohibited pattern in the static regex checks (along
with ActivityTestRule since that's deprecated and discouraged, anyway).
The test was suffering from some proto encoding inconsistencies that
seem to occur between some development machines vs. on CI. The fix
improves the test's robustness by extracting the raw encoded string,
verifying that the other outputs in the intent message correctly
correspond to that string, and that the string (as a parsed proto)
contains the correct values. As a result, the test no longer depends on
a hardcoded encoding value to be present for verification. This does
result in a bit more logic than is generally good to have in a test (and
it lengthened the test code quite a bit), but it seems necessary in this
particular case.
@oppiabot
Copy link

oppiabot bot commented Sep 21, 2023

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Sep 21, 2023
@oppiabot oppiabot bot closed this Sep 28, 2023
@BenHenning BenHenning reopened this Oct 26, 2023
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Oct 26, 2023
Copy link

oppiabot bot commented Nov 2, 2023

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Nov 2, 2023
@oppiabot oppiabot bot closed this Nov 9, 2023
@BenHenning BenHenning reopened this Jan 16, 2024
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jan 16, 2024
The main change here is ensuring that Bazel 4.0.0 is used & bzlmod
disabled in newer versions of Bazel when running operations in a test
Bazel environment.

This commit also introduces some more timing tweaks on CommandExecutor
for some tests, though these only affect very specific tests (as many
script tests directly call a script's main() function and thus don't
overwrite its executor behavior).

This commit attempted to introduce "--batch" mode to runs, but the
isolation didn't actually seem to improve stability and, instead,
substantially slowed down some of the tests.
Copy link

oppiabot bot commented Jan 29, 2024

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@BenHenning BenHenning requested review from a team as code owners February 8, 2024 01:39
@BenHenning BenHenning requested review from adhiamboperes and removed request for a team February 8, 2024 01:39
@BenHenning
Copy link
Member Author

PTAL @adhiamboperes.

@BenHenning
Copy link
Member Author

BenHenning commented Feb 8, 2024

NB: This was made to block on #5335 because, given the nature of this PR, I verify it locally by building & running all tests (which was failing without the fixes introduced in that PR). This also puts the upcoming Bazel chain in a good state for chained development.

Base automatically changed from fix-instrumentation-build-failure to develop February 8, 2024 21:54
@BenHenning BenHenning requested a review from a team as a code owner February 8, 2024 21:54
BenHenning added a commit that referenced this pull request Feb 8, 2024
## Explanation
Fixes #5334
Fixes #3618

The root issue is that ``//instrumentation`` was updated in #5098 to
depend on ``//testing``. However, the current dexer that we use in Bazel
has troubles dexing certain dependencies (including Robolectric &
Mockito), and since ``//testing`` transitively pulls in these test
dependencies, the instrumentation test binaries fail to build.

The main fix is isolating the new Firebase testing dependencies to their
own package instead of needing to pull in ``//testing``. This resulted
in some broad but trivial changes in other tests throughout the
codebase. Note that I opted for just updating ``//testing`` rather than
all affected deps lists to keep things a bit simpler (stricter deps will
fix this in a later PR as part of the Bazel chain using automation to
make things a lot simpler). This addresses issue #5334.

To keep this from happening again in the future, new smoke build tests
were added for instrumentation tests and the top-level
``//instrumentation:oppia_test`` to ensure that our
``ComputeAffectedTests`` utility correctly identifies and picks up these
tests as part of the CI run when relevant (historically, instrumentation
tests have been completely ignored in CI since they can't be run yet;
note this doesn't include the instrumentation package's unit tests since
these use Robolectric and _are_ included in CI runs). This addresses
issue #3618. Note that this was verified as working using an initial
commit to this PR that only added the new smoke tests and verifying that
``//instrumentation:oppia_test_binary_smoke_test`` is now picked up and
fails, per the log output (& CI results):

```
BAZEL_TEST_TARGETS: //instrumentation:oppia_test_binary_smoke_test
```

Finally, note that there are a couple of new test targets added under
``//domain/src/test/java/org/oppia/android/domain/auth`` since it was
noticed during the development of #5138 that these were missed in an
earlier commit to develop (and should be run in both Bazel & Gradle test
passes).

## 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
N/A -- This change only targets & fixes test-only infrastructure.
Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

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

Thanks @BenHenning! This LGTM.

Noticed a typo from an existing comment.

The refactor for assertThrows is a really nice touch, along with the regex checks. It would be bice to see some flaky tests fixed when these are removed.

Copy link

oppiabot bot commented Feb 9, 2024

Hi @BenHenning, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

@BenHenning
Copy link
Member Author

Thanks @adhiamboperes! Added the suggested fix and am now enabling auto-merge.

@BenHenning BenHenning enabled auto-merge (squash) February 14, 2024 20:24
@BenHenning BenHenning merged commit a864d5d into develop Feb 14, 2024
43 checks passed
@BenHenning BenHenning deleted the fix-platform-specific-issues branch February 14, 2024 21:13
@BenHenning BenHenning changed the title [RunAllTests] Fix #5303, #5304, #5305, #5306, #5309, part of #5307, part of #5308: Fix a variety of dev platform-specific issues [Blocked: #5335] [RunAllTests] Fix #5303, #5304, #5305, #5306, #5309, part of #5307, part of #5308: Fix a variety of dev platform-specific issues Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment