[BUG]: AssertionHelpers.assertThrows() doesn't handle AssertionErrors correctly #5303
Assignees
Labels
bug
End user-perceivable behaviors which are not desirable.
Impact: Low
Low perceived user impact (e.g. edge cases).
Work: Low
Solution is clear and broken into good-first-issue-sized chunks.
Comments
BenHenning
added
bug
BenHenning
added a commit
that referenced
this issue
Feb 14, 2024
…art of #5308: Fix a variety of dev platform-specific issues [Blocked: #5335] (#5138) ## 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: - #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. - #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). - #5305: ``model/src/main/proto/BUILD.bazel`` needed a fix due to a build warning (that only occurs when trying to build all targets). - #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. - #5307 & #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. - #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 - [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 since all of the changes in this PR are infrastructural or only affect tests. --------- Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
AssertionHelpers.assertThrows() is a utility to verify that an exception is correctly thrown during test execution. However, during the development of #5138 it was discovered that test utilities trying to catch an AssertionError will not ever fail due to how assertThrows is implemented.
Steps To Reproduce
Create a test that always throws an AssertionError and wrap that in assertThrows.
Expected Behavior
assertThrows should allow catching any exception type.
Screenshots/Videos
No response
What device/emulator are you using?
N/A -- test environment
Which Android version is your device/emulator running?
N/A -- test environment
Which version of the Oppia Android app are you using?
N/A -- test environment
Additional Context
No response
The text was updated successfully, but these errors were encountered: