Fix #5312: Make todo open check locally runnable #5315
Merged
Conversation
This file contains 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
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.
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.
This does a bunch of other small things, too, but the main difference here is introducing support for interacting with GitHub via its REST API rather than requiring the user to use the 'gh' tool locally (though the gh CLI tool is still needed for maintaining authentication access).
Tests still need to be added, and some cleanup is needed once the branch's base is adjusted.
This is a script-specific dispatcher which will allow for better asynchronous support in upcoming PRs (especially for command execution). This change serves to prepare for those changes.
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.
…ipt-execution-support Conflicts: scripts/src/java/org/oppia/android/scripts/ci/ComputeAffectedTests.kt scripts/src/javatests/org/oppia/android/scripts/ci/ComputeAffectedTestsTest.kt scripts/src/javatests/org/oppia/android/scripts/common/BazelClientTest.kt
Plus, actually makes use of the new script background dispatcher in CommandExecutorImpl to make the new wiring make more sense (though its real utility will come in a follow-up PR).
…do-check-locally-runnable
This makes the TODO open check script runnable using the script background dispatcher, and fixes some tests so that they pass now. More documentation and testing work is still needed to finalize this PR.
…ipt-execution-support
…do-check-locally-runnable
BenHenning
changed the base branch from
develop
to
introduce-better-script-execution-support
|
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. |
oppiabot
bot
added
the
stale
This was done by removing the //testing dependency and, instead, having instrumentation targets depend on the direct module within //testing that they need to build. This module & its corresponding implementation binding (and tests) needed to be moved out of //testing and into their own /firebase package.
adhiamboperes
requested changes
Mar 8, 2024
scripts/src/javatests/org/oppia/android/scripts/todo/TodoOpenCheckTest.kt
Outdated
Show resolved
Hide resolved
BenHenning
commented
Mar 14, 2024
There was a problem hiding this comment.
Updated to latest & addressed comments. PTAL @adhiamboperes.
scripts/src/javatests/org/oppia/android/scripts/todo/TodoOpenCheckTest.kt
Outdated
Show resolved
Hide resolved
adhiamboperes
approved these changes
Mar 15, 2024
|
Unassigning @adhiamboperes since they have already approved the PR. |
|
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! |
Base automatically changed from
introduce-better-script-execution-support
to
develop
March 15, 2024 01:43
BenHenning
added a commit
that referenced
this pull request
Mar 15, 2024
…port (#5313) ## Explanation Fixes part of #5312 Fixes part of #59 This PR helps prepare for changes coming in #5315 and #4929 (the latter of which is the start of the main upcoming Bazel migration PR chain) by introducing one main scripts-based change: ``ScriptBackgroundCoroutineDispatcher``: a Kotlin coroutine dispatcher for executing asynchronous tasks in scripts that also supports proper Java executor service shutdown (so that scripts don't hang). This dispatcher is multi-threaded to help simplify executing large numbers of parallel background tasks. All scripts have been migrated over to running their primary operations within the context of this new dispatcher. Relevant script utilities have been updated to use it, including ``CommandExecutor`` (though this is mainly a placeholder change for the main executor changes which are coming in #4929). Miscellaneous details to note: 1. A bunch of 'e.g.' typos were fixed in ``GenerateMavenDependenciesList.kt`` and ``wiki/Updating-Maven-Dependencies.md``. These aren't functionally needed, they were just something I noticed while developing. 2. ``kotlinx-coroutines-core`` was updated from 1.4.1 to 1.4.3 in order to work around Kotlin/kotlinx.coroutines#2371 which was causing flakiness in one of the new dispatcher tests. 3. ``testClose_pendingTaskLongerThanCloseTimeout_taskIsNotRun`` intentionally takes ~2 seconds to run in order to provide some assurance that, without cancellation, the task _would_ run and the test _would_ fail (this has been manually verified in a few different situations of the dispatcher and/or test changing; some changes won't result in a failure due to how cancellation works internally for executor service & the converted coroutine dispatcher). Note that historically these changes were originally part of #4929, but they were split out so that they could be used by #5315 (which ended up being convenient to include prior to #4929). ## 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 PR doesn't include any user-facing changes since it only impacts scripts. --------- Co-authored-by: Adhiambo Peres <[email protected]>
…e-better-script-execution-support
…do-check-locally-runnable
|
Since this has been fully approved, #5313 is merged, and this PR's branch is up-to-date with develop, enabling auto-merge. |
BenHenning
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Fixes #5312
This updates the TODO open check script to be locally runnable rather than requiring the developer to manually download the list of issues from GitHub to analyze. This simplifies things and allows the script to be easily run within the
static_checks.shscript. This is being done via interacting directly with GitHub using its RESTful API (see https://docs.github.com/en/rest/issues/issues?apiVersion=2022-11-28#list-repository-issues) in conjunction with the user's local auth token used to set up theirghtool (which needs to be set up, hence the changes to the wiki documentation and clear error messages from the newGitHubClientutility).To further simplify things, a
regeneratemode was added to regenerate the TODO exemptions textproto file (which is helpful for #4929 hence why this comes before that PR).The new command syntax to perform the TODO check is:
With a specific example to just perform the check:
bazel run //scripts:todo_open_check -- $(pwd) scripts/assets/todo_open_exemptions.pbAnd an example to also perform regeneration:
bazel run //scripts:todo_open_check -- $(pwd) scripts/assets/todo_open_exemptions.pb regenerateSome other things specifically to note:
static_checks.sh.GitHubClient.GitHubClientis designed to download all issues regardless of length (whereas before the manual download step was limited to the first 2000 issues of the repository) using pagination.GitHubIssueis exempted from tests since it's just a basic data structure, so there's no specific logic to test.GitHubServiceis exempted from tests since it's a template to generate code via Retrofit's annotation processor, so there's no specific logic to test.TodoOpenCheck's output has been simplified to be relative to the repository root rather than using absolute paths (for parity with many other app scripts).GitHubClient's tests required interacting withgh, a newFakeCommandExecutorwas added (along with its own tests) to provide support for orchestrating local utilities. This may be useful in other tests in the future, though some of those script tests intentionally integrate with environment commands likegitandbazel.Essential Checklist
For UI-specific PRs only
This is an infrastructure-only PR.