From 5963eaa79609205e10d3bfd11112ed4e9e740820 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 19 Mar 2024 17:28:00 -0700 Subject: [PATCH] Fix #5312: Make todo open check locally runnable [Blocked: #5313] (#5315) ## 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.sh`` script. 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 their ``gh`` tool (which needs to be set up, hence the changes to the wiki documentation and clear error messages from the new ``GitHubClient`` utility). To further simplify things, a ``regenerate`` mode 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: ```sh bazel run //scripts:todo_open_check -- [regenerate] ``` With a specific example to just perform the check: ```sh bazel run //scripts:todo_open_check -- $(pwd) scripts/assets/todo_open_exemptions.pb ``` And an example to also perform regeneration: ```sh bazel run //scripts:todo_open_check -- $(pwd) scripts/assets/todo_open_exemptions.pb regenerate ``` Some other things specifically to note: - TODO exemptions needed to be updated in this PR due to TODO utility & test changes. The new file was created using the new regenerate functionality. - The TODO check has been added to the end of ``static_checks.sh``. - The GitHub CI workflow was updated to use the new script syntax appropriately. - This is the first time scripts have been updated to integrate with Retrofit, and this setup is going to be reused in the future for other services. - The data model for issues has been updated to better represent the remote data structure. - Moshi is being used along with Retrofit for an easier interaction with GitHub as a remote endpoint. All of this has been wrapped in a new ``GitHubClient``. - ``GitHubClient`` is 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. - New tests were added to verify the regenerate flow (and properly set up the mock OkHttp server since the script now relies on an HTTP endpoint to download the GitHub issues itself). - ``GitHubIssue`` is exempted from tests since it's just a basic data structure, so there's no specific logic to test. - ``GitHubService`` is exempted from tests since it's a template to generate code via Retrofit's annotation processor, so there's no specific logic to test. - All scripts proto libraries were updated to use normal Java (rather than Java lite) generation to provide text format support. - The file paths included in ``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). - Since ``GitHubClient``'s tests required interacting with ``gh``, a new ``FakeCommandExecutor`` was 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 like ``git`` and ``bazel``. ## 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 is an infrastructure-only PR. --------- Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> Co-authored-by: Sean Lip --- .github/workflows/static_checks.yml | 3 +- scripts/assets/test_file_exemptions.textproto | 2 + scripts/assets/todo_open_exemptions.textproto | 488 ++++++++------- .../org/oppia/android/scripts/ci/BUILD.bazel | 4 +- .../oppia/android/scripts/common/BUILD.bazel | 18 + .../android/scripts/common/GitHubClient.kt | 110 ++++ .../android/scripts/common/model/BUILD.bazel | 13 + .../scripts/common/model/GitHubIssue.kt | 13 + .../android/scripts/common/remote/BUILD.bazel | 16 + .../scripts/common/remote/GitHubService.kt | 40 ++ .../scripts/common/testing/BUILD.bazel | 15 + .../common/testing/FakeCommandExecutor.kt | 163 +++++ .../oppia/android/scripts/docs/BUILD.bazel | 2 +- .../oppia/android/scripts/label/BUILD.bazel | 2 +- .../oppia/android/scripts/license/BUILD.bazel | 1 - .../oppia/android/scripts/maven/BUILD.bazel | 1 - .../oppia/android/scripts/proto/BUILD.bazel | 33 +- .../oppia/android/scripts/regex/BUILD.bazel | 4 +- .../android/scripts/testfile/BUILD.bazel | 2 +- .../oppia/android/scripts/todo/BUILD.bazel | 5 +- .../android/scripts/todo/TodoCollector.kt | 18 +- .../scripts/todo/TodoIssueResolvedCheck.kt | 33 +- .../android/scripts/todo/TodoOpenCheck.kt | 436 ++++++------- .../android/scripts/todo/model/BUILD.bazel | 8 - .../oppia/android/scripts/todo/model/Issue.kt | 14 - .../oppia/android/scripts/todo/model/Todo.kt | 6 +- .../oppia/android/scripts/common/BUILD.bazel | 12 + .../scripts/common/GitHubClientTest.kt | 252 ++++++++ .../scripts/common/testing/BUILD.bazel | 15 + .../common/testing/FakeCommandExecutorTest.kt | 277 +++++++++ .../oppia/android/scripts/todo/BUILD.bazel | 2 + .../android/scripts/todo/TodoCollectorTest.kt | 138 +++-- .../todo/TodoIssueResolvedCheckTest.kt | 30 +- .../android/scripts/todo/TodoOpenCheckTest.kt | 576 +++++++++++++++--- scripts/static_checks.sh | 7 + wiki/Static-Analysis-Checks.md | 25 +- 36 files changed, 2099 insertions(+), 685 deletions(-) create mode 100644 scripts/src/java/org/oppia/android/scripts/common/GitHubClient.kt create mode 100644 scripts/src/java/org/oppia/android/scripts/common/model/BUILD.bazel create mode 100644 scripts/src/java/org/oppia/android/scripts/common/model/GitHubIssue.kt create mode 100644 scripts/src/java/org/oppia/android/scripts/common/remote/BUILD.bazel create mode 100644 scripts/src/java/org/oppia/android/scripts/common/remote/GitHubService.kt create mode 100644 scripts/src/java/org/oppia/android/scripts/common/testing/BUILD.bazel create mode 100644 scripts/src/java/org/oppia/android/scripts/common/testing/FakeCommandExecutor.kt delete mode 100644 scripts/src/java/org/oppia/android/scripts/todo/model/Issue.kt create mode 100644 scripts/src/javatests/org/oppia/android/scripts/common/GitHubClientTest.kt create mode 100644 scripts/src/javatests/org/oppia/android/scripts/common/testing/BUILD.bazel create mode 100644 scripts/src/javatests/org/oppia/android/scripts/common/testing/FakeCommandExecutorTest.kt diff --git a/.github/workflows/static_checks.yml b/.github/workflows/static_checks.yml index d8469d7a1d4..cd99def2b9d 100644 --- a/.github/workflows/static_checks.yml +++ b/.github/workflows/static_checks.yml @@ -177,8 +177,7 @@ jobs: env: GITHUB_TOKEN: ${{ github.token }} run: | - gh issue list --limit 2000 --repo oppia/oppia-android --json number > $(pwd)/open_issues.json - bazel run //scripts:todo_open_check -- $(pwd) scripts/assets/todo_open_exemptions.pb open_issues.json + bazel run //scripts:todo_open_check -- $(pwd) scripts/assets/todo_open_exemptions.pb - name: String Resource Validation Check if: always() diff --git a/scripts/assets/test_file_exemptions.textproto b/scripts/assets/test_file_exemptions.textproto index ee5df60d570..e2bc3b9b559 100644 --- a/scripts/assets/test_file_exemptions.textproto +++ b/scripts/assets/test_file_exemptions.textproto @@ -747,6 +747,8 @@ exempted_file_path: "instrumentation/src/java/org/oppia/android/instrumentation/ exempted_file_path: "instrumentation/src/java/org/oppia/android/instrumentation/testing/EndToEndTestHelper.kt" exempted_file_path: "scripts/src/java/org/oppia/android/scripts/common/CommandExecutor.kt" exempted_file_path: "scripts/src/java/org/oppia/android/scripts/common/CommandResult.kt" +exempted_file_path: "scripts/src/java/org/oppia/android/scripts/common/model/GitHubIssue.kt" +exempted_file_path: "scripts/src/java/org/oppia/android/scripts/common/remote/GitHubService.kt" exempted_file_path: "scripts/src/java/org/oppia/android/scripts/license/LicenseFetcher.kt" exempted_file_path: "scripts/src/java/org/oppia/android/scripts/license/LicenseFetcherImpl.kt" exempted_file_path: "scripts/src/java/org/oppia/android/scripts/license/model/CopyrightLicense.kt" diff --git a/scripts/assets/todo_open_exemptions.textproto b/scripts/assets/todo_open_exemptions.textproto index 8b201928380..f7dbbc0cdb9 100644 --- a/scripts/assets/todo_open_exemptions.textproto +++ b/scripts/assets/todo_open_exemptions.textproto @@ -1,40 +1,15 @@ todo_open_exemption { - exempted_file_path: "scripts/src/javatests/org/oppia/android/scripts/todo/TodoOpenCheckTest.kt" - line_number: 65 - line_number: 66 - line_number: 72 - line_number: 74 - line_number: 96 - line_number: 97 - line_number: 98 - line_number: 99 - line_number: 100 - line_number: 132 - line_number: 133 - line_number: 136 - line_number: 167 - line_number: 168 - line_number: 169 - line_number: 174 - line_number: 176 - line_number: 213 - line_number: 214 - line_number: 215 - line_number: 220 - line_number: 222 - line_number: 228 - line_number: 265 - line_number: 269 - line_number: 307 - line_number: 308 - line_number: 312 - line_number: 359 - line_number: 360 - line_number: 361 - line_number: 365 + exempted_file_path: "scripts/src/java/org/oppia/android/scripts/todo/TodoCollector.kt" + line_number: 78 +} +todo_open_exemption { + exempted_file_path: "scripts/src/java/org/oppia/android/scripts/todo/model/Todo.kt" + line_number: 12 } todo_open_exemption { exempted_file_path: "scripts/src/javatests/org/oppia/android/scripts/todo/TodoCollectorTest.kt" + line_number: 31 + line_number: 32 line_number: 33 line_number: 34 line_number: 35 @@ -58,127 +33,127 @@ todo_open_exemption { line_number: 53 line_number: 54 line_number: 55 - line_number: 56 - line_number: 57 - line_number: 63 - line_number: 64 - line_number: 73 - line_number: 76 - line_number: 80 - line_number: 83 - line_number: 87 - line_number: 90 - line_number: 94 - line_number: 97 - line_number: 101 - line_number: 104 - line_number: 108 - line_number: 111 - line_number: 115 - line_number: 118 - line_number: 122 - line_number: 125 - line_number: 129 - line_number: 132 - line_number: 136 - line_number: 139 - line_number: 143 - line_number: 146 - line_number: 150 - line_number: 153 - line_number: 157 - line_number: 160 - line_number: 164 - line_number: 167 - line_number: 171 - line_number: 174 - line_number: 178 - line_number: 181 - line_number: 185 - line_number: 188 - line_number: 192 - line_number: 195 - line_number: 199 - line_number: 202 - line_number: 206 - line_number: 209 - line_number: 213 - line_number: 216 - line_number: 220 - line_number: 223 - line_number: 227 - line_number: 230 - line_number: 234 - line_number: 237 - line_number: 241 - line_number: 244 - line_number: 248 - line_number: 255 - line_number: 258 - line_number: 262 - line_number: 265 - line_number: 274 - line_number: 275 - line_number: 279 - line_number: 283 - line_number: 284 - line_number: 296 - line_number: 299 - line_number: 303 - line_number: 306 - line_number: 310 - line_number: 313 - line_number: 317 - line_number: 320 - line_number: 324 - line_number: 327 + line_number: 61 + line_number: 62 + line_number: 71 + line_number: 74 + line_number: 78 + line_number: 81 + line_number: 85 + line_number: 88 + line_number: 92 + line_number: 95 + line_number: 99 + line_number: 102 + line_number: 106 + line_number: 109 + line_number: 113 + line_number: 116 + line_number: 120 + line_number: 123 + line_number: 127 + line_number: 130 + line_number: 134 + line_number: 137 + line_number: 141 + line_number: 144 + line_number: 148 + line_number: 151 + line_number: 155 + line_number: 158 + line_number: 162 + line_number: 165 + line_number: 169 + line_number: 172 + line_number: 176 + line_number: 179 + line_number: 183 + line_number: 186 + line_number: 190 + line_number: 193 + line_number: 197 + line_number: 200 + line_number: 204 + line_number: 207 + line_number: 211 + line_number: 214 + line_number: 218 + line_number: 221 + line_number: 225 + line_number: 228 + line_number: 232 + line_number: 235 + line_number: 239 + line_number: 242 + line_number: 246 + line_number: 253 + line_number: 256 + line_number: 260 + line_number: 263 + line_number: 272 + line_number: 273 + line_number: 277 + line_number: 281 + line_number: 282 + line_number: 294 + line_number: 297 + line_number: 301 + line_number: 304 + line_number: 308 + line_number: 311 + line_number: 315 + line_number: 318 + line_number: 322 + line_number: 325 + line_number: 334 + line_number: 335 line_number: 336 line_number: 337 - line_number: 338 - line_number: 339 - line_number: 344 - line_number: 349 - line_number: 352 - line_number: 365 - line_number: 368 - line_number: 372 - line_number: 375 - line_number: 379 - line_number: 382 - line_number: 386 - line_number: 389 - line_number: 393 - line_number: 396 - line_number: 400 - line_number: 403 - line_number: 407 - line_number: 410 + line_number: 342 + line_number: 347 + line_number: 350 + line_number: 363 + line_number: 366 + line_number: 370 + line_number: 373 + line_number: 377 + line_number: 380 + line_number: 384 + line_number: 387 + line_number: 391 + line_number: 394 + line_number: 398 + line_number: 401 + line_number: 405 + line_number: 408 + line_number: 417 + line_number: 418 line_number: 419 line_number: 420 - line_number: 421 - line_number: 422 - line_number: 426 - line_number: 430 - line_number: 433 - line_number: 446 - line_number: 449 - line_number: 453 - line_number: 456 - line_number: 460 - line_number: 463 - line_number: 467 - line_number: 470 - line_number: 474 - line_number: 477 - line_number: 481 - line_number: 484 - line_number: 488 - line_number: 491 + line_number: 424 + line_number: 428 + line_number: 431 + line_number: 444 + line_number: 447 + line_number: 451 + line_number: 454 + line_number: 458 + line_number: 461 + line_number: 465 + line_number: 468 + line_number: 472 + line_number: 475 + line_number: 479 + line_number: 482 + line_number: 486 + line_number: 489 + line_number: 530 + line_number: 531 line_number: 532 - line_number: 533 - line_number: 534 - line_number: 539 - line_number: 544 + line_number: 537 + line_number: 542 + line_number: 545 + line_number: 546 line_number: 547 line_number: 548 line_number: 549 @@ -200,62 +175,60 @@ todo_open_exemption { line_number: 565 line_number: 566 line_number: 567 - line_number: 568 - line_number: 569 - line_number: 575 - line_number: 588 - line_number: 591 - line_number: 595 - line_number: 598 - line_number: 602 - line_number: 605 - line_number: 609 - line_number: 612 - line_number: 616 - line_number: 619 - line_number: 627 - line_number: 636 - line_number: 645 - line_number: 654 - line_number: 663 - line_number: 672 - line_number: 681 - line_number: 690 - line_number: 699 - line_number: 708 - line_number: 717 - line_number: 726 - line_number: 735 - line_number: 744 - line_number: 753 - line_number: 762 - line_number: 771 - line_number: 780 - line_number: 789 - line_number: 798 - line_number: 807 - line_number: 816 + line_number: 573 + line_number: 586 + line_number: 589 + line_number: 593 + line_number: 596 + line_number: 600 + line_number: 603 + line_number: 607 + line_number: 610 + line_number: 614 + line_number: 617 + line_number: 625 + line_number: 634 + line_number: 643 + line_number: 652 + line_number: 661 + line_number: 670 + line_number: 679 + line_number: 688 + line_number: 697 + line_number: 706 + line_number: 715 + line_number: 724 + line_number: 733 + line_number: 742 + line_number: 751 + line_number: 760 + line_number: 769 + line_number: 778 + line_number: 787 + line_number: 796 + line_number: 805 + line_number: 814 + line_number: 824 + line_number: 825 line_number: 826 - line_number: 827 - line_number: 828 - line_number: 836 - line_number: 839 - line_number: 843 - line_number: 846 - line_number: 850 - line_number: 853 + line_number: 834 + line_number: 837 + line_number: 841 + line_number: 844 + line_number: 848 + line_number: 851 + line_number: 860 + line_number: 861 line_number: 862 - line_number: 863 - line_number: 864 - line_number: 873 - line_number: 876 - line_number: 880 - line_number: 883 - line_number: 887 - line_number: 890 + line_number: 871 + line_number: 874 + line_number: 878 + line_number: 881 + line_number: 885 + line_number: 888 + line_number: 897 + line_number: 898 line_number: 899 - line_number: 900 - line_number: 901 } todo_open_exemption { exempted_file_path: "scripts/src/javatests/org/oppia/android/scripts/todo/TodoIssueResolvedCheckTest.kt" @@ -279,31 +252,108 @@ todo_open_exemption { line_number: 192 } todo_open_exemption { - exempted_file_path: "scripts/src/java/org/oppia/android/scripts/todo/TodoCollector.kt" - line_number: 82 + exempted_file_path: "scripts/src/javatests/org/oppia/android/scripts/todo/TodoOpenCheckTest.kt" + line_number: 67 + line_number: 68 + line_number: 74 + line_number: 76 + line_number: 93 + line_number: 94 + line_number: 95 + line_number: 96 + line_number: 97 + line_number: 126 + line_number: 127 + line_number: 130 + line_number: 158 + line_number: 159 + line_number: 160 + line_number: 165 + line_number: 167 + line_number: 201 + line_number: 202 + line_number: 203 + line_number: 208 + line_number: 210 + line_number: 216 + line_number: 250 + line_number: 254 + line_number: 287 + line_number: 288 + line_number: 292 + line_number: 336 + line_number: 337 + line_number: 338 + line_number: 342 + line_number: 411 + line_number: 412 + line_number: 418 + line_number: 420 + line_number: 438 + line_number: 439 + line_number: 440 + line_number: 441 + line_number: 442 + line_number: 457 + line_number: 458 + line_number: 461 + line_number: 476 + line_number: 477 + line_number: 478 + line_number: 479 + line_number: 480 + line_number: 481 + line_number: 482 + line_number: 485 + line_number: 486 + line_number: 487 + line_number: 505 + line_number: 509 + line_number: 569 + line_number: 570 + line_number: 576 + line_number: 578 + line_number: 599 + line_number: 600 + line_number: 601 + line_number: 602 + line_number: 603 + line_number: 640 + line_number: 641 + line_number: 644 + line_number: 677 + line_number: 678 + line_number: 679 + line_number: 680 + line_number: 681 + line_number: 682 + line_number: 683 + line_number: 686 + line_number: 687 + line_number: 688 + line_number: 736 + line_number: 740 } todo_open_exemption { - exempted_file_path: "scripts/src/java/org/oppia/android/scripts/todo/model/Todo.kt" - line_number: 10 + exempted_file_path: "scripts/static_checks.sh" + line_number: 110 } - todo_open_exemption { exempted_file_path: "wiki/Coding-style-guide.md" line_number: 43 } - todo_open_exemption { exempted_file_path: "wiki/Static-Analysis-Checks.md" line_number: 171 - line_number: 179 - line_number: 184 - line_number: 188 - line_number: 192 - line_number: 194 - line_number: 209 - line_number: 219 - line_number: 222 - line_number: 225 - line_number: 228 - line_number: 231 + line_number: 195 + line_number: 200 + line_number: 204 + line_number: 208 + line_number: 210 + line_number: 232 + line_number: 242 + line_number: 245 + line_number: 248 + line_number: 251 + line_number: 254 } diff --git a/scripts/src/java/org/oppia/android/scripts/ci/BUILD.bazel b/scripts/src/java/org/oppia/android/scripts/ci/BUILD.bazel index 2e91f4a052e..f8b345eaa80 100644 --- a/scripts/src/java/org/oppia/android/scripts/ci/BUILD.bazel +++ b/scripts/src/java/org/oppia/android/scripts/ci/BUILD.bazel @@ -15,7 +15,7 @@ kt_jvm_library( "//scripts/src/java/org/oppia/android/scripts/common:bazel_client", "//scripts/src/java/org/oppia/android/scripts/common:git_client", "//scripts/src/java/org/oppia/android/scripts/common:proto_string_encoder", - "//scripts/src/java/org/oppia/android/scripts/proto:affected_tests_java_proto_lite", + "//scripts/src/java/org/oppia/android/scripts/proto:affected_tests_java_proto", ], ) @@ -28,6 +28,6 @@ kt_jvm_library( visibility = ["//scripts:oppia_script_binary_visibility"], deps = [ "//scripts/src/java/org/oppia/android/scripts/common:proto_string_encoder", - "//scripts/src/java/org/oppia/android/scripts/proto:affected_tests_java_proto_lite", + "//scripts/src/java/org/oppia/android/scripts/proto:affected_tests_java_proto", ], ) diff --git a/scripts/src/java/org/oppia/android/scripts/common/BUILD.bazel b/scripts/src/java/org/oppia/android/scripts/common/BUILD.bazel index 6b26dd4f49f..b8cd2da2323 100644 --- a/scripts/src/java/org/oppia/android/scripts/common/BUILD.bazel +++ b/scripts/src/java/org/oppia/android/scripts/common/BUILD.bazel @@ -28,6 +28,24 @@ kt_jvm_library( ], ) +kt_jvm_library( + name = "github_client", + testonly = True, + srcs = [ + "GitHubClient.kt", + ], + visibility = ["//scripts:oppia_script_library_visibility"], + deps = [ + ":command_executor", + "//scripts/src/java/org/oppia/android/scripts/common/model:github_issue", + "//scripts/src/java/org/oppia/android/scripts/common/remote:github_service", + "//third_party:com_squareup_okhttp3_okhttp", + "//third_party:com_squareup_retrofit2_converter-moshi", + "//third_party:com_squareup_retrofit2_retrofit", + "//third_party:org_jetbrains_kotlinx_kotlinx-coroutines-core", + ], +) + kt_jvm_library( name = "command_executor", srcs = [ diff --git a/scripts/src/java/org/oppia/android/scripts/common/GitHubClient.kt b/scripts/src/java/org/oppia/android/scripts/common/GitHubClient.kt new file mode 100644 index 00000000000..fd7cc105037 --- /dev/null +++ b/scripts/src/java/org/oppia/android/scripts/common/GitHubClient.kt @@ -0,0 +1,110 @@ +package org.oppia.android.scripts.common + +import com.squareup.moshi.Moshi +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Deferred +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.async +import kotlinx.coroutines.withContext +import okhttp3.OkHttpClient +import org.oppia.android.scripts.common.model.GitHubIssue +import org.oppia.android.scripts.common.remote.GitHubService +import retrofit2.Retrofit +import retrofit2.converter.moshi.MoshiConverterFactory +import java.io.File +import java.io.IOException + +/** + * General utility for interfacing with a remote GitHub repository (specifically Oppia Android). + * + * Note that this utility expects 'gh' to be available in the local environment and be properly + * authenticated. + * + * @property rootDirectory the [File] corresponding to the local repository's root directory + * @property scriptBgDispatcher the dispatcher for offloading asynchronous network I/O operations + * @property commandExecutor the executor for local commands (e.g. 'gh'). This defaults to a general + * [CommandExecutorImpl] implementation that relies upon [scriptBgDispatcher] for execution. + * @property repoOwner the owner of the remote GitHub repository. This defaults to 'oppia'. + * @property repoName the name of the remote GitHub repository. This defaults to 'oppia-android'. + */ +class GitHubClient( + private val rootDirectory: File, + private val scriptBgDispatcher: ScriptBackgroundCoroutineDispatcher, + private val commandExecutor: CommandExecutor = CommandExecutorImpl(scriptBgDispatcher), + private val repoOwner: String = "oppia", + private val repoName: String = "oppia-android" +) { + private val okHttpClient by lazy { OkHttpClient.Builder().build() } + private val moshi by lazy { Moshi.Builder().build() } + private val retrofit by lazy { + Retrofit.Builder() + .baseUrl(remoteApiUrl) + .addConverterFactory(MoshiConverterFactory.create(moshi)) + .client(okHttpClient) + .build() + } + private val gitHubService by lazy { retrofit.create(GitHubService::class.java) } + private val authorizationBearer by lazy { "Bearer ${retrieveAccessToken()}" } + + /** + * Asynchronously returns all [GitHubIssue]s currently open in the Oppia Android GitHub project. + */ + fun fetchAllOpenIssuesAsync(): Deferred> { + return CoroutineScope(scriptBgDispatcher).async { + // Fetch issues one page at a time (starting at page 1) until all are found. + fetchOpenIssuesRecursive(startPageNumber = 1) + } + } + + private suspend fun fetchOpenIssuesRecursive(startPageNumber: Int): List { + val issues = fetchOpenIssues(startPageNumber).await() + return if (issues.isNotEmpty()) { + issues + fetchOpenIssuesRecursive(startPageNumber + 1) + } else issues + } + + private fun fetchOpenIssues(pageNumber: Int): Deferred> { + return CoroutineScope(scriptBgDispatcher).async { + val call = gitHubService.fetchOpenIssues(repoOwner, repoName, authorizationBearer, pageNumber) + // Deferred blocking I/O operation to the dedicated I/O dispatcher. + val response = withContext(Dispatchers.IO) { call.execute() } + check(response.isSuccessful()) { + "Failed to fetch issues at page $pageNumber: ${response.code()}\n${call.request()}" + + "\n${response.errorBody()}." + } + return@async checkNotNull(response.body()) { + "No issues response from GitHub for page: $pageNumber." + } + } + } + + private fun retrieveAccessToken(): String { + // First, make sure the command actually exists. + try { + commandExecutor.executeCommand(rootDirectory, "gh", "help") + } catch (e: IOException) { + throw IllegalStateException( + "Failed to interact with gh tool. Please make sure your environment is set up properly" + + " per https://github.com/oppia/oppia-android/wiki/Static-Analysis-Checks" + + "#todo-open-checks.", + e + ) + } + + // Retrieve the access token that 'gh' is configured to use (to allow the script to run without + // being tied to a specific access token). + return commandExecutor.executeCommand(rootDirectory, "gh", "auth", "token").also { + check(it.exitCode == 0) { + "Failed to retrieve auth token from GH tool. Please make sure your environment is set up" + + " properly per https://github.com/oppia/oppia-android/wiki/Static-Analysis-Checks" + + "#todo-open-checks. Command output:\n${it.output.joinToString(separator = "\n")}" + } + }.output.single() + } + + companion object { + // TODO(#5314): Migrate this over to a Dagger constant. + /** The remote URL corresponding to GitHub's REST API. */ + var remoteApiUrl = "https://api.github.com/" + } +} diff --git a/scripts/src/java/org/oppia/android/scripts/common/model/BUILD.bazel b/scripts/src/java/org/oppia/android/scripts/common/model/BUILD.bazel new file mode 100644 index 00000000000..af3cc0b52d7 --- /dev/null +++ b/scripts/src/java/org/oppia/android/scripts/common/model/BUILD.bazel @@ -0,0 +1,13 @@ +""" +Data structures corresponding to common utilities, such as for GitHub API integration. +""" + +load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_jvm_library") + +kt_jvm_library( + name = "github_issue", + testonly = True, + srcs = ["GitHubIssue.kt"], + visibility = ["//scripts:oppia_script_library_visibility"], + deps = ["//third_party:moshi"], +) diff --git a/scripts/src/java/org/oppia/android/scripts/common/model/GitHubIssue.kt b/scripts/src/java/org/oppia/android/scripts/common/model/GitHubIssue.kt new file mode 100644 index 00000000000..7a824fc3120 --- /dev/null +++ b/scripts/src/java/org/oppia/android/scripts/common/model/GitHubIssue.kt @@ -0,0 +1,13 @@ +package org.oppia.android.scripts.common.model + +import com.squareup.moshi.Json +import com.squareup.moshi.JsonClass + +/** + * Moshi data structure representing a remote issue on GitHub. + * + * @property number the unique number corresponding to this issue (i.e. the number listed after + * 'issues/' in an issue's GitHub URL) + */ +@JsonClass(generateAdapter = true) +data class GitHubIssue(@Json(name = "number") val number: Int) diff --git a/scripts/src/java/org/oppia/android/scripts/common/remote/BUILD.bazel b/scripts/src/java/org/oppia/android/scripts/common/remote/BUILD.bazel new file mode 100644 index 00000000000..fec47cca77b --- /dev/null +++ b/scripts/src/java/org/oppia/android/scripts/common/remote/BUILD.bazel @@ -0,0 +1,16 @@ +""" +Remote service endpoints for common shared utilities, such as for GitHub API integration. +""" + +load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_jvm_library") + +kt_jvm_library( + name = "github_service", + testonly = True, + srcs = ["GitHubService.kt"], + visibility = ["//scripts:oppia_script_library_visibility"], + deps = [ + "//scripts/src/java/org/oppia/android/scripts/common/model:github_issue", + "//third_party:com_squareup_retrofit2_retrofit", + ], +) diff --git a/scripts/src/java/org/oppia/android/scripts/common/remote/GitHubService.kt b/scripts/src/java/org/oppia/android/scripts/common/remote/GitHubService.kt new file mode 100644 index 00000000000..0fdb1dd3a19 --- /dev/null +++ b/scripts/src/java/org/oppia/android/scripts/common/remote/GitHubService.kt @@ -0,0 +1,40 @@ +package org.oppia.android.scripts.common.remote + +import org.oppia.android.scripts.common.model.GitHubIssue +import retrofit2.Call +import retrofit2.http.GET +import retrofit2.http.Header +import retrofit2.http.Headers +import retrofit2.http.Path +import retrofit2.http.Query + +/** Retrofit service for interacting with GitHub's REST API. */ +interface GitHubService { + + /** + * Fetches [countPerPage] open issues from the configured GitHub repository. + * + * Note that the returned issues are sorted in ascending order by creation date as a basic attempt + * to improve API stability (e.g. robustness against new issues being filed when fetching multiple + * pages of issues) since the GitHub API doesn't seem to provide support for stable pagination. + * + * API reference: + * https://docs.github.com/en/rest/issues/issues?apiVersion=2022-11-28#list-repository-issues. + * + * @param repoOwner the owner of the repository to be read (e.g. 'oppia') + * @param repoName the name of the repository to be read (e.g. 'oppia-android') + * @param authorizationBearer the authorization access token bearer to allow repository reading + * @param pageNumber the current page number to read (starting at 1) + * @param countPerPage the number of issues to read per page (defaults to 100) + * @return the list of [GitHubIssue]s read from the remote repository (as a [Call]) + */ + @Headers("Accept: application/vnd.github+json", "X-GitHub-Api-Version: 2022-11-28") + @GET("repos/{repo_owner}/{repo_name}/issues?direction=asc") + fun fetchOpenIssues( + @Path("repo_owner") repoOwner: String, + @Path("repo_name") repoName: String, + @Header("Authorization") authorizationBearer: String, + @Query("page") pageNumber: Int, + @Query("per_page") countPerPage: Int = 100 + ): Call> +} diff --git a/scripts/src/java/org/oppia/android/scripts/common/testing/BUILD.bazel b/scripts/src/java/org/oppia/android/scripts/common/testing/BUILD.bazel new file mode 100644 index 00000000000..8e1428bfaee --- /dev/null +++ b/scripts/src/java/org/oppia/android/scripts/common/testing/BUILD.bazel @@ -0,0 +1,15 @@ +""" +Package for test-only utilities that correspond to common script utilities. +""" + +load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_jvm_library") + +kt_jvm_library( + name = "fake_command_executor", + testonly = True, + srcs = ["FakeCommandExecutor.kt"], + visibility = ["//scripts:oppia_script_test_visibility"], + deps = [ + "//scripts/src/java/org/oppia/android/scripts/common:command_executor", + ], +) diff --git a/scripts/src/java/org/oppia/android/scripts/common/testing/FakeCommandExecutor.kt b/scripts/src/java/org/oppia/android/scripts/common/testing/FakeCommandExecutor.kt new file mode 100644 index 00000000000..2485f3ac8fb --- /dev/null +++ b/scripts/src/java/org/oppia/android/scripts/common/testing/FakeCommandExecutor.kt @@ -0,0 +1,163 @@ +package org.oppia.android.scripts.common.testing + +import org.oppia.android.scripts.common.CommandExecutor +import org.oppia.android.scripts.common.CommandResult +import java.io.ByteArrayOutputStream +import java.io.Closeable +import java.io.File +import java.io.IOException +import java.io.PrintStream +import java.util.concurrent.ConcurrentHashMap + +/** + * Test-only fake [CommandExecutor] that can be orchestrated to avoid introducing a real dependency + * on the local runtime environment. + * + * This executor works by delegating incoming commands to registered [CommandHandler]s (via + * [registerHandler]). If no handler exists for a specific command, an [IOException] is thrown + * (since the command can't be found) for parity with a production implementation of + * [CommandExecutor]. + */ +class FakeCommandExecutor : CommandExecutor { + private val handlers = ConcurrentHashMap() + + override fun executeCommand( + workingDir: File, + command: String, + vararg arguments: String, + includeErrorOutput: Boolean + ): CommandResult { + val handler = handlers[command] ?: DefaultCommandHandler + val argList = arguments.toList() + return OutputLogger.createLogger(ignoreErrorOutput = !includeErrorOutput).use { logger -> + CommandResult( + exitCode = handler.handleCommand(command, argList, logger.outputStream, logger.errorStream), + output = logger.outputLines, + errorOutput = logger.errorLines, + command = listOf(command) + argList + ) + } + } + + /** Registers a new [CommandHandler] for the specified [command]. */ + fun registerHandler(command: String, handler: CommandHandler) { + handlers[command] = handler + } + + /** Registers a new [CommandHandler] for the specified [command]. */ + fun registerHandler( + command: String, + handle: (String, List, PrintStream, PrintStream) -> Int + ) { + val handler = object : CommandHandler { + override fun handleCommand( + command: String, + args: List, + outputStream: PrintStream, + errorStream: PrintStream + ): Int = handle(command, args, outputStream, errorStream) + } + registerHandler(command, handler) + } + + // TODO(#4122): Convert this to a fun interface & remove the second registerHandler method above. + /** Handles commands that come to a [FakeCommandExecutor] via [executeCommand]. */ + interface CommandHandler { + /** + * Handles the request to execute a command. + * + * @param command the specific, case-sensitive command that's to be executed + * @param args the list of arguments to pass to the command + * @param outputStream where standard output for the command should be printed + * @param errorStream where error output for the command should be printed + * @return the status code of the command's execution + */ + fun handleCommand( + command: String, + args: List, + outputStream: PrintStream, + errorStream: PrintStream + ): Int + } + + /** + * [Closeable] logger for tracking standard & error output in [CommandHandler]s. + * + * New instances can be created using [createLogger]. + * + * Loggers should be [close]d prior to consuming [outputLines] or [errorLines], otherwise some + * lines may not be included. + */ + private sealed class OutputLogger : Closeable { + /** + * The destination for standard output from the [CommandHandler], only if this logger isn't + * [close]d. + */ + abstract val outputStream: PrintStream + + /** + * The destination for error output from the [CommandHandler], only if this logger isn't + * [close]d. + */ + abstract val errorStream: PrintStream + + /** The tracked standard output lines from the [CommandHandler]. */ + abstract val outputLines: List + + /** The tracked error output lines from the [CommandHandler]. */ + abstract val errorLines: List + + override fun close() { + outputStream.close() + errorStream.close() + } + + /** [OutputLogger] that only tracks standard output and ignores error output. */ + protected class DropErrorsLogger : OutputLogger() { + override val outputStream by lazy { PrintStream(outStreamBufferTracker) } + override val errorStream by lazy { PrintStream(errStreamBufferTracker) } + override val outputLines get() = outStreamBufferString.split("\n") + override val errorLines = emptyList() + + private val outStreamBufferTracker by lazy { ByteArrayOutputStream() } + private val errStreamBufferTracker by lazy { ByteArrayOutputStream() } + private val outStreamBufferString + get() = outStreamBufferTracker.toByteArray().toString(Charsets.UTF_8) + } + + /** [OutputLogger] that only tracks both standard and error ouput. */ + protected class SplitLogger : OutputLogger() { + override val outputStream by lazy { PrintStream(outStreamBufferTracker) } + override val errorStream by lazy { PrintStream(errStreamBufferTracker) } + override val outputLines get() = outStreamBufferString.split("\n") + override val errorLines get() = errStreamBufferString.split("\n") + + private val outStreamBufferTracker by lazy { ByteArrayOutputStream() } + private val errStreamBufferTracker by lazy { ByteArrayOutputStream() } + private val outStreamBufferString + get() = outStreamBufferTracker.toByteArray().toString(Charsets.UTF_8) + private val errStreamBufferString + get() = errStreamBufferTracker.toByteArray().toString(Charsets.UTF_8) + } + + companion object { + /** + * Returns a new [OutputLogger] that either tracks standard and error output, or only standard + * output (ignoring error output) depending on the provided [ignoreErrorOutput] parameter. + */ + fun createLogger(ignoreErrorOutput: Boolean): OutputLogger = + if (ignoreErrorOutput) DropErrorsLogger() else SplitLogger() + } + } + + private companion object { + private object DefaultCommandHandler : CommandHandler { + override fun handleCommand( + command: String, + args: List, + outputStream: PrintStream, + errorStream: PrintStream + ): Int = throw IOException("Command doesn't exist.") + } + } +} diff --git a/scripts/src/java/org/oppia/android/scripts/docs/BUILD.bazel b/scripts/src/java/org/oppia/android/scripts/docs/BUILD.bazel index f342da5ec2b..72ff0c07474 100644 --- a/scripts/src/java/org/oppia/android/scripts/docs/BUILD.bazel +++ b/scripts/src/java/org/oppia/android/scripts/docs/BUILD.bazel @@ -11,7 +11,7 @@ kt_jvm_library( visibility = ["//scripts:oppia_script_binary_visibility"], deps = [ "//scripts/src/java/org/oppia/android/scripts/common:repository_file", - "//scripts/src/java/org/oppia/android/scripts/proto:script_exemptions_java_proto_lite", + "//scripts/src/java/org/oppia/android/scripts/proto:script_exemptions_java_proto", "//third_party:org_jetbrains_kotlin_kotlin-compiler-embeddable", ], ) diff --git a/scripts/src/java/org/oppia/android/scripts/label/BUILD.bazel b/scripts/src/java/org/oppia/android/scripts/label/BUILD.bazel index 01524932353..7ad9a92714b 100644 --- a/scripts/src/java/org/oppia/android/scripts/label/BUILD.bazel +++ b/scripts/src/java/org/oppia/android/scripts/label/BUILD.bazel @@ -12,6 +12,6 @@ kt_jvm_library( visibility = ["//scripts:oppia_script_binary_visibility"], deps = [ "//scripts/src/java/org/oppia/android/scripts/common:repository_file", - "//scripts/src/java/org/oppia/android/scripts/proto:script_exemptions_java_proto_lite", + "//scripts/src/java/org/oppia/android/scripts/proto:script_exemptions_java_proto", ], ) diff --git a/scripts/src/java/org/oppia/android/scripts/license/BUILD.bazel b/scripts/src/java/org/oppia/android/scripts/license/BUILD.bazel index aaeeb5d34a6..67e3d13032e 100644 --- a/scripts/src/java/org/oppia/android/scripts/license/BUILD.bazel +++ b/scripts/src/java/org/oppia/android/scripts/license/BUILD.bazel @@ -24,7 +24,6 @@ kt_jvm_library( "//scripts/src/java/org/oppia/android/scripts/common:bazel_client", "//scripts/src/java/org/oppia/android/scripts/maven/model", "//scripts/src/java/org/oppia/android/scripts/proto:maven_dependencies_java_proto", - "//scripts/src/java/org/oppia/android/scripts/proto:maven_dependencies_java_proto_lite", "//third_party:com_google_protobuf_protobuf-java", ], ) diff --git a/scripts/src/java/org/oppia/android/scripts/maven/BUILD.bazel b/scripts/src/java/org/oppia/android/scripts/maven/BUILD.bazel index 189bc4e2be9..91261ac00db 100644 --- a/scripts/src/java/org/oppia/android/scripts/maven/BUILD.bazel +++ b/scripts/src/java/org/oppia/android/scripts/maven/BUILD.bazel @@ -24,6 +24,5 @@ kt_jvm_library( "//scripts/src/java/org/oppia/android/scripts/license/model", "//scripts/src/java/org/oppia/android/scripts/maven/model", "//scripts/src/java/org/oppia/android/scripts/proto:maven_dependencies_java_proto", - "//scripts/src/java/org/oppia/android/scripts/proto:maven_dependencies_java_proto_lite", ], ) diff --git a/scripts/src/java/org/oppia/android/scripts/proto/BUILD.bazel b/scripts/src/java/org/oppia/android/scripts/proto/BUILD.bazel index 60f6588f1a0..7265ff0efc3 100644 --- a/scripts/src/java/org/oppia/android/scripts/proto/BUILD.bazel +++ b/scripts/src/java/org/oppia/android/scripts/proto/BUILD.bazel @@ -1,12 +1,13 @@ """ This library contains all protos used in the scripts module. -In Bazel, proto files are built using the oppia_proto_library() and java_lite_proto_library() rules. -The oppia_proto_library() rule creates a proto file library to be used in multiple languages. -The java_lite_proto_library() rule takes in a proto_library target and generates java code. -For more context on adding a new proto library, please refer to model/BUILD + +In Bazel, proto files are built using the oppia_proto_library() and java_proto_library() rules. The +oppia_proto_library() rule creates a proto file library to be used in multiple languages. The +java_proto_library() rule takes in a proto_library target and generates java code. For more context +on adding a new proto library, please refer to //model/BUILD.bazel. """ -load("@rules_java//java:defs.bzl", "java_lite_proto_library", "java_proto_library") +load("@rules_java//java:defs.bzl", "java_proto_library") load("//model:oppia_proto_library.bzl", "oppia_proto_library") oppia_proto_library( @@ -14,8 +15,8 @@ oppia_proto_library( srcs = ["affected_tests.proto"], ) -java_lite_proto_library( - name = "affected_tests_java_proto_lite", +java_proto_library( + name = "affected_tests_java_proto", visibility = ["//scripts:oppia_script_library_visibility"], deps = [":affected_tests_proto"], ) @@ -26,8 +27,8 @@ oppia_proto_library( visibility = ["//scripts:oppia_script_binary_visibility"], ) -java_lite_proto_library( - name = "filename_pattern_validation_checks_java_proto_lite", +java_proto_library( + name = "filename_pattern_validation_checks_java_proto", visibility = ["//scripts:oppia_script_library_visibility"], deps = [":filename_pattern_validation_checks_proto"], ) @@ -38,8 +39,8 @@ oppia_proto_library( visibility = ["//scripts:oppia_script_binary_visibility"], ) -java_lite_proto_library( - name = "file_content_validation_checks_java_proto_lite", +java_proto_library( + name = "file_content_validation_checks_java_proto", visibility = ["//scripts:oppia_script_library_visibility"], deps = [":file_content_validation_checks_proto"], ) @@ -50,8 +51,8 @@ oppia_proto_library( visibility = ["//scripts:oppia_script_binary_visibility"], ) -java_lite_proto_library( - name = "script_exemptions_java_proto_lite", +java_proto_library( + name = "script_exemptions_java_proto", visibility = ["//scripts:oppia_script_library_visibility"], deps = [":script_exemptions_proto"], ) @@ -67,9 +68,3 @@ java_proto_library( visibility = ["//scripts:oppia_script_library_visibility"], deps = [":maven_dependencies_proto"], ) - -java_lite_proto_library( - name = "maven_dependencies_java_proto_lite", - visibility = ["//scripts:oppia_script_library_visibility"], - deps = [":maven_dependencies_proto"], -) diff --git a/scripts/src/java/org/oppia/android/scripts/regex/BUILD.bazel b/scripts/src/java/org/oppia/android/scripts/regex/BUILD.bazel index 3b9ca212aa1..82867e55ee1 100644 --- a/scripts/src/java/org/oppia/android/scripts/regex/BUILD.bazel +++ b/scripts/src/java/org/oppia/android/scripts/regex/BUILD.bazel @@ -12,7 +12,7 @@ kt_jvm_library( visibility = ["//scripts:oppia_script_binary_visibility"], deps = [ "//scripts/src/java/org/oppia/android/scripts/common:repository_file", - "//scripts/src/java/org/oppia/android/scripts/proto:file_content_validation_checks_java_proto_lite", - "//scripts/src/java/org/oppia/android/scripts/proto:filename_pattern_validation_checks_java_proto_lite", + "//scripts/src/java/org/oppia/android/scripts/proto:file_content_validation_checks_java_proto", + "//scripts/src/java/org/oppia/android/scripts/proto:filename_pattern_validation_checks_java_proto", ], ) diff --git a/scripts/src/java/org/oppia/android/scripts/testfile/BUILD.bazel b/scripts/src/java/org/oppia/android/scripts/testfile/BUILD.bazel index dbcb1023446..6777296ae26 100644 --- a/scripts/src/java/org/oppia/android/scripts/testfile/BUILD.bazel +++ b/scripts/src/java/org/oppia/android/scripts/testfile/BUILD.bazel @@ -12,6 +12,6 @@ kt_jvm_library( visibility = ["//scripts:oppia_script_binary_visibility"], deps = [ "//scripts/src/java/org/oppia/android/scripts/common:repository_file", - "//scripts/src/java/org/oppia/android/scripts/proto:script_exemptions_java_proto_lite", + "//scripts/src/java/org/oppia/android/scripts/proto:script_exemptions_java_proto", ], ) diff --git a/scripts/src/java/org/oppia/android/scripts/todo/BUILD.bazel b/scripts/src/java/org/oppia/android/scripts/todo/BUILD.bazel index a099106c2cb..351184690d0 100644 --- a/scripts/src/java/org/oppia/android/scripts/todo/BUILD.bazel +++ b/scripts/src/java/org/oppia/android/scripts/todo/BUILD.bazel @@ -23,8 +23,9 @@ kt_jvm_library( visibility = ["//scripts:oppia_script_binary_visibility"], deps = [ ":todo_collector", - "//scripts/src/java/org/oppia/android/scripts/proto:script_exemptions_java_proto_lite", - "//scripts/src/java/org/oppia/android/scripts/todo/model:issue", + "//scripts/src/java/org/oppia/android/scripts/common:github_client", + "//scripts/src/java/org/oppia/android/scripts/common/model:github_issue", + "//scripts/src/java/org/oppia/android/scripts/proto:script_exemptions_java_proto", "//scripts/src/java/org/oppia/android/scripts/todo/model:todo", ], ) diff --git a/scripts/src/java/org/oppia/android/scripts/todo/TodoCollector.kt b/scripts/src/java/org/oppia/android/scripts/todo/TodoCollector.kt index d7b8271f544..a4487f1396a 100644 --- a/scripts/src/java/org/oppia/android/scripts/todo/TodoCollector.kt +++ b/scripts/src/java/org/oppia/android/scripts/todo/TodoCollector.kt @@ -2,6 +2,7 @@ package org.oppia.android.scripts.todo import org.oppia.android.scripts.common.RepositoryFile import org.oppia.android.scripts.todo.model.Todo +import java.io.File /** Collects code lines containing the 'todo' keyword (case-insensitive). */ class TodoCollector { @@ -26,11 +27,7 @@ class TodoCollector { file.bufferedReader() .lineSequence() .mapIndexedNotNull { lineIndex, lineContent -> - checkIfContainsTodo( - filePath = file.toString(), - lineContent = lineContent, - lineIndex = lineIndex - ) + checkIfContainsTodo(file.absoluteFile.normalize(), lineContent, lineIndex) } } } @@ -65,21 +62,20 @@ class TodoCollector { * @param codeLine the line of code to be checked * @return the parsed issue number */ - fun parseIssueNumberFromTodo(codeLine: String): String? { - return correctTodoFormatRegex.find(codeLine)?.groupValues?.get(1) - } + fun parseIssueNumberFromTodo(codeLine: String): Int? = + correctTodoFormatRegex.find(codeLine)?.groupValues?.get(1)?.toIntOrNull() /** * Computes whether a line of code contains the 'todo' keyword. * - * @param filePath the path of the file + * @param file the file being checked * @param lineContent the line string * @param lineIndex the index of the line sequence which is to be searched for a TODO * @return a Todo instance if the todo detector regex matches, else returns null */ - private fun checkIfContainsTodo(filePath: String, lineContent: String, lineIndex: Int): Todo? { + private fun checkIfContainsTodo(file: File, lineContent: String, lineIndex: Int): Todo? { if (todoDetectorRegex.containsMatchIn(lineContent)) { - return Todo(filePath = filePath, lineNumber = lineIndex + 1, lineContent = lineContent) + return Todo(file, lineNumber = lineIndex + 1, lineContent = lineContent) } return null } diff --git a/scripts/src/java/org/oppia/android/scripts/todo/TodoIssueResolvedCheck.kt b/scripts/src/java/org/oppia/android/scripts/todo/TodoIssueResolvedCheck.kt index d0f9a1db331..18ce0d201c7 100644 --- a/scripts/src/java/org/oppia/android/scripts/todo/TodoIssueResolvedCheck.kt +++ b/scripts/src/java/org/oppia/android/scripts/todo/TodoIssueResolvedCheck.kt @@ -22,11 +22,12 @@ import java.io.File * NOTE TO DEVELOPERS: The script is executed in the CI enviornment. */ fun main(vararg args: String) { - // Path of the repo to be analyzed. - val repoPath = "${args[0]}/" + // The first argument is the path of the repo to be analyzed. + val repoRoot = File("${args[0]}/").absoluteFile.normalize() + val repoPath = repoRoot.path // Issue number of the closed issue. - val closedIssueNumber = args[1] + val closedIssueNumber = args[1].toInt() val commitSha = args[2] @@ -42,6 +43,7 @@ fun main(vararg args: String) { } logFailures( + repoRoot, todoIssueResolvedFailures = todoIssueResolvedFailures, failureMessage = "The following TODOs are unresolved for the closed issue:" ) @@ -54,7 +56,7 @@ fun main(vararg args: String) { } if (todoIssueResolvedFailures.isNotEmpty()) { - generateTodoListFile(repoPath, todoIssueResolvedFailures, githubPermalinkUrl) + generateTodoListFile(repoRoot, todoIssueResolvedFailures, githubPermalinkUrl) throw Exception("TODO ISSUE RESOLVED CHECK FAILED") } else { println("TODO ISSUE RESOLVED CHECK PASSED") @@ -67,7 +69,7 @@ fun main(vararg args: String) { * @param codeLine line content corresponding to the todo * @param closedIssueNumber issue number of the closed issue */ -private fun checkIfTodoIssueResolvedFailure(codeLine: String, closedIssueNumber: String): Boolean { +private fun checkIfTodoIssueResolvedFailure(codeLine: String, closedIssueNumber: Int): Boolean { val parsedIssueNumberFromTodo = TodoCollector.parseIssueNumberFromTodo(codeLine) return parsedIssueNumberFromTodo == closedIssueNumber } @@ -75,22 +77,22 @@ private fun checkIfTodoIssueResolvedFailure(codeLine: String, closedIssueNumber: /** * Generates a file containing all the todos corresponding to the closed issue. * - * @param repoPath path of the repo to be analyzed + * @param repoRoot the root directory of the repository * @param todoIssueResolvedFailures list of all the unresolved todos corresponding to the closed * issue. * @param githubPermalinkUrl the GitHub url for the permalinks */ private fun generateTodoListFile( - repoPath: String, + repoRoot: File, todoIssueResolvedFailures: List, githubPermalinkUrl: String ) { - val todoListFile = File(repoPath + "script_failures.txt") + val todoListFile = File(repoRoot, "script_failures.txt") todoListFile.appendText("The issue is reopened because of the following unresolved TODOs:\n") - todoIssueResolvedFailures.sortedWith(compareBy({ it.filePath }, { it.lineNumber })) + todoIssueResolvedFailures.sortedWith(compareBy({ it.file.path }, { it.lineNumber })) .forEach { todo -> todoListFile.appendText( - "$githubPermalinkUrl/${(todo.filePath).removePrefix(repoPath)}#L${todo.lineNumber}\n" + "$githubPermalinkUrl/${(todo.file.toRelativeString(repoRoot))}#L${todo.lineNumber}\n" ) } } @@ -98,14 +100,19 @@ private fun generateTodoListFile( /** * Logs the TODO issue resolved check failures. * + * @param repoRoot the root directory of the repository * @param todoIssueResolvedFailures list of all the unresolved todos for the closed issue * @param failureMessage the failure message to be logged */ -private fun logFailures(todoIssueResolvedFailures: List, failureMessage: String) { +private fun logFailures( + repoRoot: File, + todoIssueResolvedFailures: List, + failureMessage: String +) { if (todoIssueResolvedFailures.isNotEmpty()) { println(failureMessage) - todoIssueResolvedFailures.sortedWith(compareBy({ it.filePath }, { it.lineNumber })).forEach { - println("- ${it.filePath}:${it.lineNumber}") + todoIssueResolvedFailures.sortedWith(compareBy({ it.file.path }, { it.lineNumber })).forEach { + println("- ${it.file.toRelativeString(repoRoot)}:${it.lineNumber}") } println() } diff --git a/scripts/src/java/org/oppia/android/scripts/todo/TodoOpenCheck.kt b/scripts/src/java/org/oppia/android/scripts/todo/TodoOpenCheck.kt index 65fc87faa0b..ddc7c2e5602 100644 --- a/scripts/src/java/org/oppia/android/scripts/todo/TodoOpenCheck.kt +++ b/scripts/src/java/org/oppia/android/scripts/todo/TodoOpenCheck.kt @@ -1,12 +1,14 @@ package org.oppia.android.scripts.todo -import com.squareup.moshi.JsonAdapter -import com.squareup.moshi.Moshi -import com.squareup.moshi.Types -import com.squareup.moshi.kotlin.reflect.KotlinJsonAdapterFactory +import com.google.protobuf.TextFormat +import kotlinx.coroutines.runBlocking +import org.oppia.android.scripts.common.CommandExecutor +import org.oppia.android.scripts.common.CommandExecutorImpl +import org.oppia.android.scripts.common.GitHubClient +import org.oppia.android.scripts.common.ScriptBackgroundCoroutineDispatcher +import org.oppia.android.scripts.common.model.GitHubIssue import org.oppia.android.scripts.proto.TodoOpenExemption import org.oppia.android.scripts.proto.TodoOpenExemptions -import org.oppia.android.scripts.todo.model.Issue import org.oppia.android.scripts.todo.model.Todo import java.io.File import java.io.FileInputStream @@ -15,250 +17,268 @@ import java.io.FileInputStream * Script for ensuring that all TODOs present in the repository are correctly formatted and * corresponds to open issues on GitHub. * + * Note that the setup instructions at + * https://github.com/oppia/oppia-android/wiki/Static-Analysis-Checks#todo-open-checks must be + * followed in order to ensure that this script works correctly in the local environment. + * * Usage: - * bazel run //scripts:todo_open_check -- - * + * bazel run //scripts:todo_open_check -- [regenerate] * * Arguments: - * - path_to_directory_root: directory path to the root of the Oppia Android repository. + * - path_to_dir_root: directory path to the root of the Oppia Android repository. * - path_to_proto_binary: relative path to the exemption .pb file. - * - path_to_json_file: path to the json file containing the list of all open issues on Github + * - regenerate: optional 'regenerate' string to, instead of checking for TODOs, regenerate the + * exemption textproto file and print it to the command output. * - * Example: + * Examples: * bazel run //scripts:todo_open_check -- $(pwd) scripts/assets/todo_open_exemptions.pb - * open_issues.json - * - * NOTE TO DEVELOPERS: The script is executed in the CI enviornment. The CI workflow creates a - * json file from the GitHub api which contains a list of all open issues of the - * oppia/oppia-android repository. To execute it without the CI, please create open issues json - * file and provide its path to the script in the format as stated above. - * - * Instructions to create the open_issues.json file: - * 1. Set up Github CLI Tools locally. - * 2. cd to the oppia-android repository. - * 3. Run the command: gh issue list --limit 2000 --repo oppia/oppia-android - * --json number > $(pwd)/open_issues.json + * bazel run //scripts:todo_open_check -- $(pwd) scripts/assets/todo_open_exemptions.pb regenerate */ fun main(vararg args: String) { - // Path of the repo to be analyzed. - val repoPath = "${args[0]}/" - + val repoRoot = File(args[0]).absoluteFile.normalize() val pathToProtoBinary = args[1] + val regenerateFile = args.getOrNull(2) == "regenerate" + ScriptBackgroundCoroutineDispatcher().use { scriptBgDispatcher -> + TodoOpenCheck(repoRoot, scriptBgDispatcher).runTodoOpenCheck(pathToProtoBinary, regenerateFile) + } +} - // Path to the JSON file containing the list of open issues. - val openIssuesJsonFile = File(repoPath, args[2]) - - check(openIssuesJsonFile.exists()) { "$repoPath${args[2]}: No such file exists" } +/** + * Utility used to determine whether TODOs in the specified repository are correctly formatted and + * correspond to open issues on GitHub. + */ +class TodoOpenCheck( + private val repoRoot: File, + private val scriptBgDispatcher: ScriptBackgroundCoroutineDispatcher, + private val commandExecutor: CommandExecutor = CommandExecutorImpl(scriptBgDispatcher) +) { + private val gitHubClient by lazy { GitHubClient(repoRoot, scriptBgDispatcher, commandExecutor) } - val todoExemptionTextProtoFilePath = "scripts/assets/todo_exemptions" + /** + * Determines whether the TODOs in the configured repository are correctly formatted and + * correspond to open issues on GitHub. + * + * @param pathToProtoBinary the absolute path to the exemptions proto that defines lines on which + * it's okay for the issue to either not exist or not be correctly formatted + * @param regenerateFile whether, regardless of an existing failure, the exemptions file should be + * regenerated and printed to the standard output in textproto format + */ + fun runTodoOpenCheck(pathToProtoBinary: String, regenerateFile: Boolean) { + // List of all the open issues on GitHub of this repository. + val openIssueList = runBlocking { gitHubClient.fetchAllOpenIssuesAsync().await() } - // List of all the open issues on GitHub of this repository. - val openIssueList = retrieveOpenIssueList(openIssuesJsonFile) + val todoExemptionList = + loadTodoExemptionsProto(pathToProtoBinary).getTodoOpenExemptionList() - val todoExemptionList = - loadTodoExemptionsProto(pathToProtoBinary).getTodoOpenExemptionList() + val allTodos = TodoCollector.collectTodos(repoPath = "${repoRoot.path}/") - val allTodos = TodoCollector.collectTodos(repoPath) + val poorlyFormattedTodos = TodoCollector.collectPoorlyFormattedTodos(allTodos) - val poorlyFormattedTodos = TodoCollector.collectPoorlyFormattedTodos(allTodos) + val correctlyFormattedTodos = TodoCollector.collectCorrectlyFormattedTodos( + allTodos - poorlyFormattedTodos + ) - val correctlyFormattedTodos = TodoCollector.collectCorrectlyFormattedTodos( - allTodos - poorlyFormattedTodos - ) + val openIssueFailureTodos = correctlyFormattedTodos.filter { todo -> + checkIfIssueDoesNotMatchOpenIssue(codeLine = todo.lineContent, openIssueList = openIssueList) + } - val openIssueFailureTodos = correctlyFormattedTodos.filter { todo -> - checkIfIssueDoesNotMatchOpenIssue(codeLine = todo.lineContent, openIssueList = openIssueList) - } + val redundantExemptions = retrieveRedundantExemptions( + todos = poorlyFormattedTodos + openIssueFailureTodos, todoExemptionList, repoRoot + ) - val redundantExemptions = retrieveRedundantExemptions( - todos = poorlyFormattedTodos + openIssueFailureTodos, - todoExemptionList = todoExemptionList, - repoPath = repoPath - ) + val poorlyFormattedTodosAfterExemption = + retrieveTodosAfterExemption(todos = poorlyFormattedTodos, todoExemptionList, repoRoot) - val poorlyFormattedTodosAfterExemption = retrieveTodosAfterExemption( - todos = poorlyFormattedTodos, - todoExemptionList = todoExemptionList, - repoPath = repoPath - ) + val openIssueFailureTodosAfterExemption = + retrieveTodosAfterExemption(todos = openIssueFailureTodos, todoExemptionList, repoRoot) - val openIssueFailureTodosAfterExemption = retrieveTodosAfterExemption( - todos = openIssueFailureTodos, - todoExemptionList = todoExemptionList, - repoPath = repoPath - ) + val todoExemptionTextProtoFilePath = "scripts/assets/todo_exemptions" + logRedundantExemptions(redundantExemptions, todoExemptionTextProtoFilePath) - logRedundantExemptions(redundantExemptions, todoExemptionTextProtoFilePath) + logFailures( + invalidTodos = poorlyFormattedTodosAfterExemption, + repoRoot, + failureMessage = "TODOs not in correct format:", + ) - logFailures( - invalidTodos = poorlyFormattedTodosAfterExemption, - failureMessage = "TODOs not in correct format:", - ) + logFailures( + invalidTodos = openIssueFailureTodosAfterExemption, + repoRoot, + failureMessage = "TODOs not corresponding to open issues on GitHub:", + ) - logFailures( - invalidTodos = openIssueFailureTodosAfterExemption, - failureMessage = "TODOs not corresponding to open issues on GitHub:", - ) + if (poorlyFormattedTodosAfterExemption.isNotEmpty() || + openIssueFailureTodosAfterExemption.isNotEmpty() + ) { + println( + "Refer to https://github.com/oppia/oppia-android/wiki/Static-Analysis-Checks" + + "#todo-open-checks for more details on how to fix this.\n" + ) + } - if (poorlyFormattedTodosAfterExemption.isNotEmpty() || - openIssueFailureTodosAfterExemption.isNotEmpty() - ) { - println( - "Refer to https://github.com/oppia/oppia-android/wiki/Static-Analysis-Checks" + - "#todo-open-checks for more details on how to fix this.\n" - ) - } + if (regenerateFile) { + println("Regenerated exemptions:") + println() + val allProblematicTodos = poorlyFormattedTodos + openIssueFailureTodos + val newExemptions = allProblematicTodos.convertToExemptions(repoRoot) + println(newExemptions.convertToExemptionTextProto()) + throw Exception("TODO CHECK SKIPPED") + } - if ( - redundantExemptions.isNotEmpty() || - poorlyFormattedTodosAfterExemption.isNotEmpty() || - openIssueFailureTodosAfterExemption.isNotEmpty() - ) { - throw Exception("TODO CHECK FAILED") - } else { - println("TODO CHECK PASSED") + if ( + redundantExemptions.isNotEmpty() || + poorlyFormattedTodosAfterExemption.isNotEmpty() || + openIssueFailureTodosAfterExemption.isNotEmpty() + ) { + println( + "There were failures. Re-run //scripts:todo_open_check with \"regenerate\" at the end" + + " to regenerate the exemption file with all failures as exempted." + ) + println() + throw Exception("TODO CHECK FAILED") + } else { + println("TODO CHECK PASSED") + } } -} -/** - * Retrieves the TODO open check failures list after filtering them from the exemptions. - * - * @param todos the list of all the failure causing TODOs - * @param todoExemptionList the list contating the TODO exemptions - * @param repoPath path of the repo to be analyzed - * @return list obtained after filtering the exemptions - */ -private fun retrieveTodosAfterExemption( - todos: List, - todoExemptionList: List, - repoPath: String -): List { - return todos.filter { todo -> - todoExemptionList.none { it -> - it.exemptedFilePath == todo.filePath.removePrefix(repoPath) && - todo.lineNumber in it.getLineNumberList() + /** + * Retrieves the TODO open check failures list after filtering them from the exemptions. + * + * @param todos the list of all the failure causing TODOs + * @param todoExemptionList the list contating the TODO exemptions + * @param repoRoot the root directory of the repository + * @return list obtained after filtering the exemptions + */ + private fun retrieveTodosAfterExemption( + todos: List, + todoExemptionList: List, + repoRoot: File + ): List { + return todos.filter { todo -> + todoExemptionList.none { + it.exemptedFilePath == todo.file.toRelativeString(repoRoot) && + todo.lineNumber in it.lineNumberList + } } } -} -/** - * Retrieves a list of redundant exemptions. - * - * @param todos the list of all the failure causing TODOs - * @param todoExemptionList the list contating the TODO exemptions - * @param repoPath path of the repo to be analyzed - * @return a list of all the redundant exemptions - */ -private fun retrieveRedundantExemptions( - todos: List, - todoExemptionList: List, - repoPath: String -): List> { - return todoExemptionList.flatMap { exemption -> - exemption.getLineNumberList().mapNotNull { exemptedLineNumber -> - val isRedundantExemption = todos.none { - it.filePath.removePrefix(repoPath) == exemption.exemptedFilePath && - it.lineNumber == exemptedLineNumber - } - if (isRedundantExemption) { - Pair(exemption.exemptedFilePath, exemptedLineNumber) - } else { - null + /** + * Retrieves a list of redundant exemptions. + * + * @param todos the list of all the failure causing TODOs + * @param todoExemptionList the list contating the TODO exemptions + * @param repoRoot the root directory of the repository + * @return a list of all the redundant exemptions + */ + private fun retrieveRedundantExemptions( + todos: List, + todoExemptionList: List, + repoRoot: File + ): List> { + return todoExemptionList.flatMap { exemption -> + exemption.lineNumberList.mapNotNull { exemptedLineNumber -> + val isRedundantExemption = todos.none { + it.file.toRelativeString(repoRoot) == exemption.exemptedFilePath && + it.lineNumber == exemptedLineNumber + } + if (isRedundantExemption) { + Pair(exemption.exemptedFilePath, exemptedLineNumber) + } else { + null + } } } } -} -/** - * Checks whether a TODO does not corresponds to open issues on GitHub. - * - * @param codeLine the line of code to be checked - * @param openIssueList the list of all the open issues of this repository on GitHub - * @return whether the TODO does not corresponds to open issues on GitHub - */ -private fun checkIfIssueDoesNotMatchOpenIssue( - codeLine: String, - openIssueList: List, -): Boolean { - val parsedIssueNumberFromTodo = TodoCollector.parseIssueNumberFromTodo(codeLine) - return openIssueList.none { it -> it.issueNumber == parsedIssueNumberFromTodo } -} + /** + * Checks whether a TODO does not corresponds to open issues on GitHub. + * + * @param codeLine the line of code to be checked + * @param openIssueList the list of all the open issues of this repository on GitHub + * @return whether the TODO does not corresponds to open issues on GitHub + */ + private fun checkIfIssueDoesNotMatchOpenIssue( + codeLine: String, + openIssueList: List, + ): Boolean { + val parsedIssueNumberFromTodo = TodoCollector.parseIssueNumberFromTodo(codeLine) + return openIssueList.none { it -> it.number == parsedIssueNumberFromTodo } + } -/** - * Logs the redundant exemptions. - * - * @param redundantExemptions list of redundant exemptions - * @param todoExemptionTextProtoFilePath the location of the TODO exemption textproto file - */ -private fun logRedundantExemptions( - redundantExemptions: List>, - todoExemptionTextProtoFilePath: String -) { - if (redundantExemptions.isNotEmpty()) { - println("Redundant exemptions (there are no TODOs corresponding to these lines):") - redundantExemptions.sortedWith(compareBy({ it.first }, { it.second })).forEach { exemption -> - println("- ${exemption.first}:${exemption.second}") + /** + * Logs the redundant exemptions. + * + * @param redundantExemptions list of redundant exemptions + * @param todoExemptionTextProtoFilePath the location of the TODO exemption textproto file + */ + private fun logRedundantExemptions( + redundantExemptions: List>, + todoExemptionTextProtoFilePath: String + ) { + if (redundantExemptions.isNotEmpty()) { + println("Redundant exemptions (there are no TODOs corresponding to these lines):") + redundantExemptions.sortedWith(compareBy({ it.first }, { it.second })).forEach { exemption -> + println("- ${exemption.first}:${exemption.second}") + } + println( + "Please remove them from $todoExemptionTextProtoFilePath.textproto" + ) + println() } - println( - "Please remove them from $todoExemptionTextProtoFilePath.textproto" - ) - println() } -} -/** - * Logs the TODO open check failures. - * - * @param invalidTodos a list of all the invalid TODOs present in the repository. A TODO is - * considered to be invalid if it is poorly formatted or if it does not corresponds to open - * issues on GitHub. - * @param failureMessage the failure message to be logged - */ -private fun logFailures(invalidTodos: List, failureMessage: String) { - if (invalidTodos.isNotEmpty()) { - println(failureMessage) - invalidTodos.sortedWith(compareBy({ it.filePath }, { it.lineNumber })).forEach { - println("- ${it.filePath}:${it.lineNumber}") + /** + * Logs the TODO open check failures. + * + * @param invalidTodos a list of all the invalid TODOs present in the repository. A TODO is + * considered to be invalid if it is poorly formatted or if it does not corresponds to open + * issues on GitHub. + * @param repoRoot the root directory of the repository + * @param failureMessage the failure message to be logged + */ + private fun logFailures(invalidTodos: List, repoRoot: File, failureMessage: String) { + if (invalidTodos.isNotEmpty()) { + println(failureMessage) + invalidTodos.sortedWith(compareBy({ it.file.path }, { it.lineNumber })).forEach { + println("- ${it.file.toRelativeString(repoRoot)}:${it.lineNumber}") + } + println() } - println() } -} -/** - * Retrieves the list of all open issues on GitHub by parsing the JSON file generated by the GitHub - * API. - * - * @param openIssuesJsonFile file containing all the open issues of the repository - * @return list of all open issues - */ -private fun retrieveOpenIssueList(openIssuesJsonFile: File): List { - val openIssuesJsonText = openIssuesJsonFile - .inputStream() - .bufferedReader() - .use { it.readText() } - val moshi = Moshi.Builder().addLast(KotlinJsonAdapterFactory()).build() - val listType = Types.newParameterizedType(List::class.java, Issue::class.java) - val adapter: JsonAdapter> = moshi.adapter(listType) - return adapter.fromJson(openIssuesJsonText) - ?: throw Exception("Failed to parse $openIssuesJsonFile") -} + private fun List.convertToExemptions(repoRoot: File): List { + return groupBy { it.file.path }.map { (_, todos) -> + TodoOpenExemption.newBuilder().apply { + exemptedFilePath = todos.first().file.toRelativeString(repoRoot) + addAllLineNumber(todos.map { it.lineNumber }.sorted()) + }.build() + }.sortedBy { it.exemptedFilePath } + } -/** - * Loads the TODO open check exemptions list corresponding to a text proto file. - * - * @param pathToProtoBinary the location of the exemption textproto file - * @return proto class from the parsed textproto file - */ -private fun loadTodoExemptionsProto(pathToProtoBinary: String): TodoOpenExemptions { - val protoBinaryFile = File(pathToProtoBinary) - val builder = TodoOpenExemptions.getDefaultInstance().newBuilderForType() + private fun List.convertToExemptionTextProto(): String { + val baseProto = TodoOpenExemptions.newBuilder().apply { + addAllTodoOpenExemption(this@convertToExemptionTextProto) + }.build() + return TextFormat.printer().printToString(baseProto) + } + + /** + * Loads the TODO open check exemptions list corresponding to a text proto file. + * + * @param pathToProtoBinary the location of the exemption textproto file + * @return proto class from the parsed textproto file + */ + private fun loadTodoExemptionsProto(pathToProtoBinary: String): TodoOpenExemptions { + val protoBinaryFile = File(pathToProtoBinary) + val builder = TodoOpenExemptions.getDefaultInstance().newBuilderForType() - // This cast is type-safe since proto guarantees type consistency from mergeFrom(), - // and this method is bounded by the generic type T. - @Suppress("UNCHECKED_CAST") - val protoObj: TodoOpenExemptions = - FileInputStream(protoBinaryFile).use { - builder.mergeFrom(it) - }.build() as TodoOpenExemptions - return protoObj + // This cast is type-safe since proto guarantees type consistency from mergeFrom(), + // and this method is bounded by the generic type T. + @Suppress("UNCHECKED_CAST") + val protoObj: TodoOpenExemptions = + FileInputStream(protoBinaryFile).use { + builder.mergeFrom(it) + }.build() as TodoOpenExemptions + return protoObj + } } diff --git a/scripts/src/java/org/oppia/android/scripts/todo/model/BUILD.bazel b/scripts/src/java/org/oppia/android/scripts/todo/model/BUILD.bazel index 7df0565783a..34cd905db58 100644 --- a/scripts/src/java/org/oppia/android/scripts/todo/model/BUILD.bazel +++ b/scripts/src/java/org/oppia/android/scripts/todo/model/BUILD.bazel @@ -4,14 +4,6 @@ Libraries corresponding to data structures for representing a parsed open_issues load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_jvm_library") -kt_jvm_library( - name = "issue", - testonly = True, - srcs = ["Issue.kt"], - visibility = ["//scripts:oppia_script_library_visibility"], - deps = ["//third_party:moshi"], -) - kt_jvm_library( name = "todo", testonly = True, diff --git a/scripts/src/java/org/oppia/android/scripts/todo/model/Issue.kt b/scripts/src/java/org/oppia/android/scripts/todo/model/Issue.kt deleted file mode 100644 index 3ab249a844c..00000000000 --- a/scripts/src/java/org/oppia/android/scripts/todo/model/Issue.kt +++ /dev/null @@ -1,14 +0,0 @@ -package org.oppia.android.scripts.todo.model - -import com.squareup.moshi.Json -import com.squareup.moshi.JsonClass - -/** - * Data class that contains the required details of an [Issue] that is present in - * open_issues.json. - */ -@JsonClass(generateAdapter = true) -data class Issue( - /** The issue's identification number. */ - @Json(name = "number") val issueNumber: String, -) diff --git a/scripts/src/java/org/oppia/android/scripts/todo/model/Todo.kt b/scripts/src/java/org/oppia/android/scripts/todo/model/Todo.kt index 4a4bd66dae3..855ade490a4 100644 --- a/scripts/src/java/org/oppia/android/scripts/todo/model/Todo.kt +++ b/scripts/src/java/org/oppia/android/scripts/todo/model/Todo.kt @@ -1,10 +1,12 @@ package org.oppia.android.scripts.todo.model +import java.io.File + /** * Represents the structure of TODO. * - * @property filePath the path of the file + * @property file the file containing a TODO * @property lineNumber the line number of the line of code * @property lineContent the content of the line of code */ -data class Todo(val filePath: String, val lineNumber: Int, val lineContent: String) +data class Todo(val file: File, val lineNumber: Int, val lineContent: String) diff --git a/scripts/src/javatests/org/oppia/android/scripts/common/BUILD.bazel b/scripts/src/javatests/org/oppia/android/scripts/common/BUILD.bazel index bb2009d98bd..03d24a11d19 100644 --- a/scripts/src/javatests/org/oppia/android/scripts/common/BUILD.bazel +++ b/scripts/src/javatests/org/oppia/android/scripts/common/BUILD.bazel @@ -40,6 +40,18 @@ kt_jvm_test( ], ) +kt_jvm_test( + name = "GitHubClientTest", + srcs = ["GitHubClientTest.kt"], + deps = [ + "//scripts/src/java/org/oppia/android/scripts/common:github_client", + "//scripts/src/java/org/oppia/android/scripts/common/testing:fake_command_executor", + "//testing:assertion_helpers", + "//third_party:com_google_truth_truth", + "//third_party:com_squareup_okhttp3_mockwebserver", + ], +) + kt_jvm_test( name = "ProtoStringEncoderTest", srcs = ["ProtoStringEncoderTest.kt"], diff --git a/scripts/src/javatests/org/oppia/android/scripts/common/GitHubClientTest.kt b/scripts/src/javatests/org/oppia/android/scripts/common/GitHubClientTest.kt new file mode 100644 index 00000000000..112fba48291 --- /dev/null +++ b/scripts/src/javatests/org/oppia/android/scripts/common/GitHubClientTest.kt @@ -0,0 +1,252 @@ +package org.oppia.android.scripts.common + +import com.google.common.truth.Truth.assertThat +import kotlinx.coroutines.runBlocking +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import org.junit.After +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder +import org.oppia.android.scripts.common.testing.FakeCommandExecutor +import org.oppia.android.testing.assertThrows +import java.lang.IllegalStateException + +/** Tests for [GitHubClient]. */ +// Function name: test names are conventionally named with underscores. +@Suppress("FunctionName") +class GitHubClientTest { + private companion object { + private const val TEST_AUTH_TOKEN = "abcdef1234567890" + } + + @field:[Rule JvmField] val tempFolder = TemporaryFolder() + + private val scriptBgDispatcher by lazy { ScriptBackgroundCoroutineDispatcher() } + private val fakeCommandExecutor by lazy { FakeCommandExecutor() } + private lateinit var mockWebServer: MockWebServer + + @Before + fun setUp() { + mockWebServer = MockWebServer() + GitHubClient.remoteApiUrl = mockWebServer.url("/").toString() + } + + @After + fun tearDown() { + scriptBgDispatcher.close() + } + + @Test + fun testFetchAllOpenIssuesAsync_noGhTool_throwsException() { + setGitHubServiceNextResponseWithNoIssues() + val gitHubClient = GitHubClient(tempFolder.root, scriptBgDispatcher, fakeCommandExecutor) + + val exception = assertThrows() { + runBlocking { gitHubClient.fetchAllOpenIssuesAsync().await() } + } + + assertThat(exception).hasMessageThat().contains("Failed to interact with gh tool.") + } + + @Test + fun testFetchAllOpenIssuesAsync_ghTool_missingAuthToken_throwsException() { + setUpSupportForGhAuthWithMissingToken() + setGitHubServiceNextResponseWithNoIssues() + val gitHubClient = GitHubClient(tempFolder.root, scriptBgDispatcher, fakeCommandExecutor) + + val exception = assertThrows() { + runBlocking { gitHubClient.fetchAllOpenIssuesAsync().await() } + } + + assertThat(exception).hasMessageThat().contains("Failed to retrieve auth token from GH tool.") + } + + @Test + fun testFetchAllOpenIssuesAsync_withInvalidAuthToken_throwsException() { + setUpSupportForGhAuth(authToken = "Invalid") + setGitHubServiceNextResponseWithFailureCode(errorCode = 401) // Simulate invalid auth token. + val gitHubClient = GitHubClient(tempFolder.root, scriptBgDispatcher, fakeCommandExecutor) + + val exception = assertThrows() { + runBlocking { gitHubClient.fetchAllOpenIssuesAsync().await() } + } + + assertThat(exception).hasMessageThat().contains("Failed to fetch issues at page") + } + + @Test + fun testFetchAllOpenIssuesAsync_withAuthToken_requestHasCorrectPath() { + setUpSupportForGhAuth(authToken = TEST_AUTH_TOKEN) + setGitHubServiceNextResponseWithNoIssues() + val gitHubClient = GitHubClient(tempFolder.root, scriptBgDispatcher, fakeCommandExecutor) + + runBlocking { gitHubClient.fetchAllOpenIssuesAsync().await() } + + val request = mockWebServer.takeRequest() + assertThat(request.requestUrl?.encodedPath).isEqualTo("/repos/oppia/oppia-android/issues") + } + + @Test + fun testFetchAllOpenIssuesAsync_withAuthToken_requestIncludesAuthorizationBearer() { + setUpSupportForGhAuth(authToken = TEST_AUTH_TOKEN) + setGitHubServiceNextResponseWithNoIssues() + val gitHubClient = GitHubClient(tempFolder.root, scriptBgDispatcher, fakeCommandExecutor) + + runBlocking { gitHubClient.fetchAllOpenIssuesAsync().await() } + + val request = mockWebServer.takeRequest() + assertThat(request.getHeader("Authorization")).isEqualTo("Bearer $TEST_AUTH_TOKEN") + } + + @Test + fun testFetchAllOpenIssuesAsync_withAuthToken_requestIncludesDataFormat() { + setUpSupportForGhAuth(authToken = TEST_AUTH_TOKEN) + setGitHubServiceNextResponseWithNoIssues() + val gitHubClient = GitHubClient(tempFolder.root, scriptBgDispatcher, fakeCommandExecutor) + + runBlocking { gitHubClient.fetchAllOpenIssuesAsync().await() } + + val request = mockWebServer.takeRequest() + assertThat(request.getHeader("Accept")).isEqualTo("application/vnd.github+json") + } + + @Test + fun testFetchAllOpenIssuesAsync_withAuthToken_requestIncludesApiVersion() { + setUpSupportForGhAuth(authToken = TEST_AUTH_TOKEN) + setGitHubServiceNextResponseWithNoIssues() + val gitHubClient = GitHubClient(tempFolder.root, scriptBgDispatcher, fakeCommandExecutor) + + runBlocking { gitHubClient.fetchAllOpenIssuesAsync().await() } + + val request = mockWebServer.takeRequest() + assertThat(request.getHeader("X-GitHub-Api-Version")).isEqualTo("2022-11-28") + } + + @Test + fun testFetchAllOpenIssuesAsync_withAuthToken_requestsQueriesIssuesInAscendingOrder() { + setUpSupportForGhAuth(authToken = TEST_AUTH_TOKEN) + setGitHubServiceNextResponseWithNoIssues() + val gitHubClient = GitHubClient(tempFolder.root, scriptBgDispatcher, fakeCommandExecutor) + + runBlocking { gitHubClient.fetchAllOpenIssuesAsync().await() } + + val request = mockWebServer.takeRequest() + assertThat(request.requestUrl?.queryParameter("direction")).isEqualTo("asc") + } + + @Test + fun testFetchAllOpenIssuesAsync_withAuthToken_missingResourceResponse_throwsException() { + setUpSupportForGhAuth(authToken = TEST_AUTH_TOKEN) + setGitHubServiceNextResponseWithFailureCode(errorCode = 404) + val gitHubClient = GitHubClient(tempFolder.root, scriptBgDispatcher, fakeCommandExecutor) + + val exception = assertThrows() { + runBlocking { gitHubClient.fetchAllOpenIssuesAsync().await() } + } + + assertThat(exception).hasMessageThat().contains("Failed to fetch issues at page") + } + + @Test + fun testFetchAllOpenIssuesAsync_withAuthToken_nullBody_throwsException() { + setUpSupportForGhAuth(authToken = TEST_AUTH_TOKEN) + setGitHubServiceNextResponseWithNullResponse() + val gitHubClient = GitHubClient(tempFolder.root, scriptBgDispatcher, fakeCommandExecutor) + + val exception = assertThrows() { + runBlocking { gitHubClient.fetchAllOpenIssuesAsync().await() } + } + + assertThat(exception).hasMessageThat().contains("No issues response from GitHub for page") + } + + @Test + fun testFetchAllOpenIssuesAsync_withAuthToken_noIssues_returnEmptyList() { + setUpSupportForGhAuth(authToken = TEST_AUTH_TOKEN) + setGitHubServiceNextResponseWithNoIssues() + val gitHubClient = GitHubClient(tempFolder.root, scriptBgDispatcher, fakeCommandExecutor) + + val openIssues = runBlocking { gitHubClient.fetchAllOpenIssuesAsync().await() } + + assertThat(openIssues).isEmpty() + } + + @Test + fun testFetchAllOpenIssuesAsync_withAuthToken_someIssues_returnIssuesList() { + setUpSupportForGhAuth(authToken = TEST_AUTH_TOKEN) + setGitHubServiceNextResponseWithSinglePageOfIssues(11, 57) + val gitHubClient = GitHubClient(tempFolder.root, scriptBgDispatcher, fakeCommandExecutor) + + val openIssues = runBlocking { gitHubClient.fetchAllOpenIssuesAsync().await() } + + assertThat(openIssues).hasSize(2) + assertThat(openIssues[0].number).isEqualTo(11) + assertThat(openIssues[1].number).isEqualTo(57) + } + + @Test + fun testFetchAllOpenIssuesAsync_withAuthToken_multiplePagesOfIssues_returnsAllIssues() { + setUpSupportForGhAuth(authToken = TEST_AUTH_TOKEN) + setGitHubServiceNextResponseWithPagesOfIssues(listOf(11, 57), listOf(42), listOf(77, 28)) + val gitHubClient = GitHubClient(tempFolder.root, scriptBgDispatcher, fakeCommandExecutor) + + val openIssues = runBlocking { gitHubClient.fetchAllOpenIssuesAsync().await() } + + assertThat(openIssues).hasSize(5) + assertThat(openIssues[0].number).isEqualTo(11) + assertThat(openIssues[1].number).isEqualTo(57) + assertThat(openIssues[2].number).isEqualTo(42) + assertThat(openIssues[3].number).isEqualTo(77) + assertThat(openIssues[4].number).isEqualTo(28) + } + + private fun setUpSupportForGhAuthWithMissingToken() { + fakeCommandExecutor.registerHandler("gh") { _, args, _, errorStream -> + when (args) { + listOf("help") -> 0 + listOf("auth", "token") -> 1.also { errorStream.println("No auth token configured.") } + else -> 1 + } + } + } + + private fun setUpSupportForGhAuth(authToken: String) { + fakeCommandExecutor.registerHandler("gh") { _, args, outputStream, _ -> + when (args) { + listOf("help") -> 0 + listOf("auth", "token") -> 0.also { outputStream.print(authToken) } + else -> 1 + } + } + } + + private fun setGitHubServiceNextResponseWithNoIssues() { + setGitHubServiceNextResponseWithSinglePageOfIssues(/* no issues */) + } + + private fun setGitHubServiceNextResponseWithSinglePageOfIssues(vararg issueNumbers: Int) { + setGitHubServiceNextResponseWithPagesOfIssues(issueNumbers.toList()) + } + + private fun setGitHubServiceNextResponseWithPagesOfIssues(vararg issuePages: List) { + issuePages.forEach { issueNumbers -> + val issueJsons = issueNumbers.joinToString(separator = ",") { "{\"number\":$it}" } + setGitHubServiceNextResponseWithJsonResponse("[$issueJsons]") + } + setGitHubServiceNextResponseWithJsonResponse("[]") // No more issues. + } + + private fun setGitHubServiceNextResponseWithNullResponse() { + setGitHubServiceNextResponseWithJsonResponse(rawJson = "null") + } + + private fun setGitHubServiceNextResponseWithJsonResponse(rawJson: String) { + mockWebServer.enqueue(MockResponse().setBody(rawJson)) + } + + private fun setGitHubServiceNextResponseWithFailureCode(errorCode: Int) { + mockWebServer.enqueue(MockResponse().setResponseCode(errorCode)) + } +} diff --git a/scripts/src/javatests/org/oppia/android/scripts/common/testing/BUILD.bazel b/scripts/src/javatests/org/oppia/android/scripts/common/testing/BUILD.bazel new file mode 100644 index 00000000000..f81195ad233 --- /dev/null +++ b/scripts/src/javatests/org/oppia/android/scripts/common/testing/BUILD.bazel @@ -0,0 +1,15 @@ +""" +Tests corresponding to test-only utilities that correspond to common script utilities. +""" + +load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_jvm_test") + +kt_jvm_test( + name = "FakeCommandExecutorTest", + srcs = ["FakeCommandExecutorTest.kt"], + deps = [ + "//scripts/src/java/org/oppia/android/scripts/common/testing:fake_command_executor", + "//testing:assertion_helpers", + "//third_party:com_google_truth_truth", + ], +) diff --git a/scripts/src/javatests/org/oppia/android/scripts/common/testing/FakeCommandExecutorTest.kt b/scripts/src/javatests/org/oppia/android/scripts/common/testing/FakeCommandExecutorTest.kt new file mode 100644 index 00000000000..406103f3a15 --- /dev/null +++ b/scripts/src/javatests/org/oppia/android/scripts/common/testing/FakeCommandExecutorTest.kt @@ -0,0 +1,277 @@ +package org.oppia.android.scripts.common.testing + +import com.google.common.truth.Truth.assertThat +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder +import org.oppia.android.testing.assertThrows +import java.io.IOException + +/** Tests for [FakeCommandExecutor]. */ +// Function name: test names are conventionally named with underscores. +@Suppress("FunctionName") +class FakeCommandExecutorTest { + @field:[Rule JvmField] val tempFolder = TemporaryFolder() + + @Test + fun testRegisterCommand_doesNotThrowException() { + val commandExecutor = FakeCommandExecutor() + + commandExecutor.registerHandler("test") { _, _, _, _ -> 0 } + + // The verification is that no exception is thrown. + } + + @Test + fun testRegisterTwoCommands_doesNotThrowException() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test1") { _, _, _, _ -> 0 } + + // Register a second, different command. + commandExecutor.registerHandler("test2") { _, _, _, _ -> 1 } + + // The verification is that no exception is thrown. + } + + @Test + fun testRegisterCommandTwice_doesNotThrowException() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test1") { _, _, _, _ -> 0 } + + // Register the same command a second time. + commandExecutor.registerHandler("test1") { _, _, _, _ -> 1 } + + // The verification is that no exception is thrown. + } + + @Test + fun testExecuteCommand_unregisteredCommand_throwsIoException() { + val commandExecutor = FakeCommandExecutor() + + val exception = assertThrows() { + commandExecutor.executeCommand(tempFolder.root, "test") + } + + assertThat(exception).hasMessageThat().contains("Command doesn't exist.") + } + + @Test + fun testExecuteCommand_diffCaseFromRegistered_throwsIoException() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test") { _, _, _, _ -> 0 } + + val exception = assertThrows() { + commandExecutor.executeCommand(tempFolder.root, "TEST") + } + + // Commands are case-sensitive. + assertThat(exception).hasMessageThat().contains("Command doesn't exist.") + } + + @Test + fun testExecuteCommand_registeredCommand_noArgs_returnsResultWithCorrectCommandLine() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test") { _, _, _, _ -> 0 } + + val result = commandExecutor.executeCommand(tempFolder.root, "test") + + assertThat(result.command).containsExactly("test").inOrder() + } + + @Test + fun testExecuteCommand_registeredCommand_oneArg_returnsResultWithCorrectCommandLine() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test") { _, _, _, _ -> 0 } + + val result = commandExecutor.executeCommand(tempFolder.root, "test", "arg1") + + assertThat(result.command).containsExactly("test", "arg1").inOrder() + } + + @Test + fun testExecuteCommand_registeredCommand_multipleArgs_returnsResultWithCorrectCommandLine() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test") { _, _, _, _ -> 0 } + + val result = commandExecutor.executeCommand(tempFolder.root, "test", "arg1", "arg3", "arg2") + + assertThat(result.command).containsExactly("test", "arg1", "arg3", "arg2").inOrder() + } + + @Test + fun testExecuteCommand_registeredCommand_returnsResultWithCorrectExitCode() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test") { _, _, _, _ -> 0 } + + val result = commandExecutor.executeCommand(tempFolder.root, "test") + + assertThat(result.exitCode).isEqualTo(0) + } + + @Test + fun testExecuteCommand_registeredCommand_noArgs_returnsCorrectExitCodeUsingArgs() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test") { _, args, _, _ -> 1 + args.sumOf { it.length } } + + val result = commandExecutor.executeCommand(tempFolder.root, "test") + + // No args passed. + assertThat(result.exitCode).isEqualTo(1) + } + + @Test + fun testExecuteCommand_registeredCommand_oneArg_returnsCorrectExitCodeUsingArgs() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test") { _, args, _, _ -> 1 + args.sumOf { it.length } } + + val result = commandExecutor.executeCommand(tempFolder.root, "test", "aa") + + // One arg contributes to the exit code value. + assertThat(result.exitCode).isEqualTo(3) + } + + @Test + fun testExecuteCommand_registeredCommand_multipleArgs_returnsCorrectExitCodeUsingArgs() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test") { _, args, _, _ -> 1 + args.sumOf { it.length } } + + val result = commandExecutor.executeCommand(tempFolder.root, "test", "aa", "b", "ccc") + + // All args contribute to the exit code value. + assertThat(result.exitCode).isEqualTo(7) + } + + @Test + fun testExecuteCommand_registeredCommand_returnsResultWithCorrectStandardOutput() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test") { _, _, standardStream, errorStream -> + standardStream.println("Line one") + errorStream.println("Error line one") + standardStream.println("Line two") + errorStream.println("Error line two") + errorStream.println("Error line three") + return@registerHandler 0 + } + + val result = commandExecutor.executeCommand(tempFolder.root, "test") + + assertThat(result.output).hasSize(3) + assertThat(result.output[0]).isEqualTo("Line one") + assertThat(result.output[1]).isEqualTo("Line two") + assertThat(result.output[2]).isEmpty() + } + + @Test + fun testExecuteCommand_registeredCommand_returnsResultWithCorrectErrorOutput() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test") { _, _, standardStream, errorStream -> + standardStream.println("Line one") + errorStream.println("Error line one") + standardStream.println("Line two") + errorStream.println("Error line two") + errorStream.println("Error line three") + return@registerHandler 0 + } + + val result = commandExecutor.executeCommand(tempFolder.root, "test") + + assertThat(result.errorOutput).hasSize(4) + assertThat(result.errorOutput[0]).isEqualTo("Error line one") + assertThat(result.errorOutput[1]).isEqualTo("Error line two") + assertThat(result.errorOutput[2]).isEqualTo("Error line three") + assertThat(result.errorOutput[3]).isEmpty() + } + + @Test + fun testExecuteCommand_registeredCommand_includeErrorOutput_returnsResultWithErrorOutput() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test") { _, _, standardStream, errorStream -> + standardStream.println("Line one") + errorStream.println("Error line one") + standardStream.println("Line two") + errorStream.println("Error line two") + errorStream.println("Error line three") + return@registerHandler 0 + } + + val result = commandExecutor.executeCommand(tempFolder.root, "test", includeErrorOutput = true) + + // The result should include error output. + assertThat(result.errorOutput).hasSize(4) + assertThat(result.errorOutput[0]).isEqualTo("Error line one") + assertThat(result.errorOutput[1]).isEqualTo("Error line two") + assertThat(result.errorOutput[2]).isEqualTo("Error line three") + assertThat(result.errorOutput[3]).isEmpty() + assertThat(result.output).hasSize(3) + assertThat(result.output[0]).isEqualTo("Line one") + assertThat(result.output[1]).isEqualTo("Line two") + assertThat(result.output[2]).isEmpty() + } + + @Test + fun testExecuteCommand_registeredCommand_doNotIncludeErrorOutput_returnsResultWithNoErrorLines() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test") { _, _, standardStream, errorStream -> + standardStream.println("Line one") + errorStream.println("Error line one") + standardStream.println("Line two") + errorStream.println("Error line two") + errorStream.println("Error line three") + return@registerHandler 0 + } + + val result = commandExecutor.executeCommand(tempFolder.root, "test", includeErrorOutput = false) + + // The result should include no error output. + assertThat(result.errorOutput).isEmpty() + assertThat(result.output).hasSize(3) + assertThat(result.output[0]).isEqualTo("Line one") + assertThat(result.output[1]).isEqualTo("Line two") + assertThat(result.output[2]).isEmpty() + } + + @Test + fun testExecuteCommand_secondRegisteredCommand_returnsCorrectCommandLine() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test1") { _, _, _, _ -> 0 } + commandExecutor.registerHandler("test2") { _, _, _, _ -> 1 } + + val result = commandExecutor.executeCommand(tempFolder.root, "test2", "arg1", "arg3", "arg2") + + assertThat(result.command).containsExactly("test2", "arg1", "arg3", "arg2").inOrder() + } + + @Test + fun testExecuteCommand_secondRegisteredCommand_returnsCorrectExitCode() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test1") { _, _, _, _ -> 0 } + commandExecutor.registerHandler("test2") { _, _, _, _ -> 1 } + + val result = commandExecutor.executeCommand(tempFolder.root, "test2", "arg1", "arg3", "arg2") + + assertThat(result.exitCode).isEqualTo(1) + } + + @Test + fun testExecuteCommand_replacedRegisteredCommand_returnsCorrectCommandLine() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test1") { _, _, _, _ -> 0 } + commandExecutor.registerHandler("test1") { _, _, _, _ -> 1 } + + val result = commandExecutor.executeCommand(tempFolder.root, "test1", "arg1", "arg3", "arg2") + + assertThat(result.command).containsExactly("test1", "arg1", "arg3", "arg2").inOrder() + } + + @Test + fun testExecuteCommand_replacedRegisteredCommand_returnsCorrectExitCode() { + val commandExecutor = FakeCommandExecutor() + commandExecutor.registerHandler("test1") { _, _, _, _ -> 0 } + commandExecutor.registerHandler("test1") { _, _, _, _ -> 1 } + + val result = commandExecutor.executeCommand(tempFolder.root, "test1", "arg1", "arg3", "arg2") + + // The replaced command handler should be used, instead. + assertThat(result.exitCode).isEqualTo(1) + } +} diff --git a/scripts/src/javatests/org/oppia/android/scripts/todo/BUILD.bazel b/scripts/src/javatests/org/oppia/android/scripts/todo/BUILD.bazel index 334e07b70f1..d206ecde80e 100644 --- a/scripts/src/javatests/org/oppia/android/scripts/todo/BUILD.bazel +++ b/scripts/src/javatests/org/oppia/android/scripts/todo/BUILD.bazel @@ -19,9 +19,11 @@ kt_jvm_test( name = "TodoOpenCheckTest", srcs = ["TodoOpenCheckTest.kt"], deps = [ + "//scripts/src/java/org/oppia/android/scripts/common/testing:fake_command_executor", "//scripts/src/java/org/oppia/android/scripts/todo:todo_open_check_lib", "//testing:assertion_helpers", "//third_party:com_google_truth_truth", + "//third_party:com_squareup_okhttp3_mockwebserver", "//third_party:org_jetbrains_kotlin_kotlin-test-junit", ], ) diff --git a/scripts/src/javatests/org/oppia/android/scripts/todo/TodoCollectorTest.kt b/scripts/src/javatests/org/oppia/android/scripts/todo/TodoCollectorTest.kt index a357ee517e7..ab35976dfd2 100644 --- a/scripts/src/javatests/org/oppia/android/scripts/todo/TodoCollectorTest.kt +++ b/scripts/src/javatests/org/oppia/android/scripts/todo/TodoCollectorTest.kt @@ -9,9 +9,7 @@ import org.oppia.android.scripts.todo.model.Todo /** Tests for [TodoCollector]. */ class TodoCollectorTest { - @Rule - @JvmField - var tempFolder = TemporaryFolder() + @field:[Rule JvmField] val tempFolder = TemporaryFolder() @Before fun setUp() { @@ -71,196 +69,196 @@ class TodoCollectorTest { assertThat(collectedTodos).hasSize(28) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 3, lineContent = "# TODO(#457741): test description 1" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 4, lineContent = "# TODO (#457742): test description 2" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 5, lineContent = "# TODO(#457743) : test description 3" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 6, lineContent = "# TODO(457744): test description 4" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 7, lineContent = "// TODO(#457741)" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 8, lineContent = "// TODO(#457745):" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 9, lineContent = "// TODO(#457747) test description 5" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 10, lineContent = "// some comment which has a TODO(#12182992): some description" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 11, lineContent = "// TODO(test description 7)" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 12, lineContent = "// Todo(#4577413): test description 8" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 13, lineContent = "// Todo (#4577423): test description 9" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 14, lineContent = "// Todo(#4577433) : test description 10" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 15, lineContent = "// Todo(4577443): test description 11" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 16, lineContent = "// Todo(#4577413)" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 17, lineContent = "// Todo(#4577453):" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 18, lineContent = "// Todo(#4577473) test description 12" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 19, lineContent = "// some comment which has a Todo(#12182999): some description" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 20, lineContent = "// todo(#4577413): test description 14" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 21, lineContent = "// todo (#4577423): test description 15" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 22, lineContent = "// todo(#4577433) : test description 16" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 23, lineContent = "// todo(4577443): test description 17" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 24, lineContent = "// todo(#4577413)" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 25, lineContent = "// todo(#4577453):" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 26, lineContent = "// todo(#4577473) test description 18" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 27, lineContent = "// some comment which has a todo(#12182999): some description" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 30, lineContent = "todo" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 33, lineContent = "TODO(#ISSUE_NUMBER): Revert ownership to @USERNAME after YYYY-MM-DD." ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile.toString(), + file = tempFile, lineNumber = 34, lineContent = "//TODO(#161614): some another test description" ) @@ -294,35 +292,35 @@ class TodoCollectorTest { assertThat(collectedTodos).hasSize(5) assertThat(collectedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 1, lineContent = "# TODO (#121): test todo." ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 2, lineContent = "" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile2.toString(), + file = tempFile2, lineNumber = 1, lineContent = "# TODO(#10500): Test description" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile3.toString(), + file = tempFile3, lineNumber = 1, lineContent = "// TODO(#17800): test todo." ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile3.toString(), + file = tempFile3, lineNumber = 2, lineContent = "// TODO( 210)" ) @@ -347,8 +345,8 @@ class TodoCollectorTest { val testContent3 = """ // TODO (#178): test todo. - - + + # TODO( 210) """.trimIndent() val tempFile1 = tempFolder.newFile("testfiles/TempFile1.txt") @@ -363,49 +361,49 @@ class TodoCollectorTest { assertThat(poorlyFormattedTodos).hasSize(7) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 1, lineContent = "//TODO(#1215545): test todo." ) ) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 2, lineContent = "# TODO( 110)" ) ) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 3, lineContent = "//todo(#15444)" ) ) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 4, lineContent = "" ) ) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile2.toString(), + file = tempFile2, lineNumber = 2, lineContent = "TODO(# 105)" ) ) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile3.toString(), + file = tempFile3, lineNumber = 1, lineContent = "// TODO (#178): test todo." ) ) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile3.toString(), + file = tempFile3, lineNumber = 4, lineContent = "# TODO( 210)" ) @@ -428,8 +426,8 @@ class TodoCollectorTest { val testContent3 = """ // ToDo(#17878788): test content 6 - - + + # some todo(#21084884): test content 7 """.trimIndent() val tempFile1 = tempFolder.newFile("testfiles/TempFile1.txt") @@ -444,49 +442,49 @@ class TodoCollectorTest { assertThat(poorlyFormattedTodos).hasSize(7) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 1, lineContent = "// Todo(#1215157): test content 1" ) ) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 2, lineContent = "# todo(#110484844): test content 2" ) ) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 3, lineContent = "// TODo(#15444): test content 3" ) ) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 4, lineContent = "" ) ) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile2.toString(), + file = tempFile2, lineNumber = 1, lineContent = "" ) ) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile3.toString(), + file = tempFile3, lineNumber = 1, lineContent = "// ToDo(#17878788): test content 6" ) ) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile3.toString(), + file = tempFile3, lineNumber = 4, lineContent = "# some todo(#21084884): test content 7" ) @@ -509,8 +507,8 @@ class TodoCollectorTest { val testContent3 = """ // Another Todo comment - - + + # some test comment including todo """.trimIndent() val tempFile1 = tempFolder.newFile("testfiles/TempFile1.txt") @@ -542,8 +540,8 @@ class TodoCollectorTest { val testContent3 = """ // TODO(#1788888): some description 5. - - + + # TODO(#210000): some description 6. // TODO (#457742): test description 2 // TODO(#457743) : test description 3 @@ -586,35 +584,35 @@ class TodoCollectorTest { assertThat(correctlyFormattedTodos).hasSize(5) assertThat(correctlyFormattedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 1, lineContent = "// TODO(#12111): some description 1." ) ) assertThat(correctlyFormattedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 2, lineContent = "# TODO(#110000): some description 2." ) ) assertThat(correctlyFormattedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 3, lineContent = "" ) ) assertThat(correctlyFormattedTodos).contains( Todo( - filePath = tempFile3.toString(), + file = tempFile3, lineNumber = 1, lineContent = "// TODO(#1788888): some description 5." ) ) assertThat(correctlyFormattedTodos).contains( Todo( - filePath = tempFile3.toString(), + file = tempFile3, lineNumber = 4, lineContent = "# TODO(#210000): some description 6." ) @@ -627,7 +625,7 @@ class TodoCollectorTest { "// TODO(#1548774): some test description." ) - assertThat(parsedIssueNumber).isEqualTo("1548774") + assertThat(parsedIssueNumber).isEqualTo(1548774) } @Test @@ -834,21 +832,21 @@ class TodoCollectorTest { assertThat(collectedTodos).hasSize(3) assertThat(collectedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 1, lineContent = "// TODO(#1234478" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 2, lineContent = "// Todo(#1234478" ) ) assertThat(collectedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 3, lineContent = "// todo(#1234478" ) @@ -871,21 +869,21 @@ class TodoCollectorTest { assertThat(poorlyFormattedTodos).hasSize(3) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 1, lineContent = "// TODO(#1234478" ) ) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 2, lineContent = "// Todo(#1234478" ) ) assertThat(poorlyFormattedTodos).contains( Todo( - filePath = tempFile1.toString(), + file = tempFile1, lineNumber = 3, lineContent = "// todo(#1234478" ) diff --git a/scripts/src/javatests/org/oppia/android/scripts/todo/TodoIssueResolvedCheckTest.kt b/scripts/src/javatests/org/oppia/android/scripts/todo/TodoIssueResolvedCheckTest.kt index 4880c092ec5..3665278c101 100644 --- a/scripts/src/javatests/org/oppia/android/scripts/todo/TodoIssueResolvedCheckTest.kt +++ b/scripts/src/javatests/org/oppia/android/scripts/todo/TodoIssueResolvedCheckTest.kt @@ -45,7 +45,7 @@ class TodoIssueResolvedCheckTest { val testContent1 = """ // Test comment 1 - + // Test comment 2 """.trimIndent() val testContent2 = @@ -69,7 +69,7 @@ class TodoIssueResolvedCheckTest { val testContent1 = """ // TODO(#169877): test description 1 - + // TODO(#1021211): test description 2 """.trimIndent() val testContent2 = @@ -93,7 +93,7 @@ class TodoIssueResolvedCheckTest { val testContent1 = """ // TODO(#169877): test description 1 - + // TODO(#1021211): test description 2 """.trimIndent() val testContent2 = @@ -113,9 +113,9 @@ class TodoIssueResolvedCheckTest { val failureMessage = """ The following TODOs are unresolved for the closed issue: - - ${retrieveTestFilesDirectoryPath()}/TempFile1.kt:1 - - ${retrieveTestFilesDirectoryPath()}/TempFile2.bazel:3 - + - TempFile1.kt:1 + - TempFile2.bazel:3 + $wikiReferenceNote """.trimIndent() assertThat(outContent.toString().trim()).isEqualTo(failureMessage) @@ -129,7 +129,7 @@ class TodoIssueResolvedCheckTest { val testContent1 = """ // TODO(#169877): test description 1 - + // TODO(#1021211): test description 2 """.trimIndent() val testContent2 = @@ -141,7 +141,7 @@ class TodoIssueResolvedCheckTest { val testContent3 = """ - + """.trimIndent() @@ -157,11 +157,11 @@ class TodoIssueResolvedCheckTest { val failureMessage = """ The following TODOs are unresolved for the closed issue: - - ${retrieveTestFilesDirectoryPath()}/TempFile1.kt:1 - - ${retrieveTestFilesDirectoryPath()}/TempFile2.bazel:3 - - ${retrieveTestFilesDirectoryPath()}/TempFile3.xml:1 - - ${retrieveTestFilesDirectoryPath()}/TempFile3.xml:4 - + - TempFile1.kt:1 + - TempFile2.bazel:3 + - TempFile3.xml:1 + - TempFile3.xml:4 + $wikiReferenceNote """.trimIndent() assertThat(outContent.toString().trim()).isEqualTo(failureMessage) @@ -175,7 +175,7 @@ class TodoIssueResolvedCheckTest { val testContent1 = """ // TODO(#169877): test description 1 - + // TODO(#1021211): test description 2 """.trimIndent() val testContent2 = @@ -187,7 +187,7 @@ class TodoIssueResolvedCheckTest { val testContent3 = """ - + """.trimIndent() diff --git a/scripts/src/javatests/org/oppia/android/scripts/todo/TodoOpenCheckTest.kt b/scripts/src/javatests/org/oppia/android/scripts/todo/TodoOpenCheckTest.kt index 6df3902ba9a..f23c2a01bb4 100644 --- a/scripts/src/javatests/org/oppia/android/scripts/todo/TodoOpenCheckTest.kt +++ b/scripts/src/javatests/org/oppia/android/scripts/todo/TodoOpenCheckTest.kt @@ -1,11 +1,16 @@ package org.oppia.android.scripts.todo import com.google.common.truth.Truth.assertThat +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer import org.junit.After import org.junit.Before import org.junit.Rule import org.junit.Test import org.junit.rules.TemporaryFolder +import org.oppia.android.scripts.common.GitHubClient +import org.oppia.android.scripts.common.ScriptBackgroundCoroutineDispatcher +import org.oppia.android.scripts.common.testing.FakeCommandExecutor import org.oppia.android.scripts.proto.TodoOpenExemption import org.oppia.android.scripts.proto.TodoOpenExemptions import org.oppia.android.testing.assertThrows @@ -15,49 +20,46 @@ import java.io.PrintStream /** Tests for [TodoOpenCheck]. */ class TodoOpenCheckTest { - private val outContent: ByteArrayOutputStream = ByteArrayOutputStream() - private val originalOut: PrintStream = System.out - private val TODO_CHECK_PASSED_OUTPUT_INDICATOR: String = "TODO CHECK PASSED" - private val TODO_SYNTAX_CHECK_FAILED_OUTPUT_INDICATOR: String = "TODO CHECK FAILED" + private companion object { + private const val TEST_AUTH_TOKEN = "abcdef1234567890" + } + + private val outContent = ByteArrayOutputStream() + private val originalOut = System.out + private val TODO_CHECK_PASSED_OUTPUT_INDICATOR = "TODO CHECK PASSED" + private val TODO_SYNTAX_CHECK_FAILED_OUTPUT_INDICATOR = "TODO CHECK FAILED" + private val TODO_SYNTAX_CHECK_SKIPPED_OUTPUT_INDICATOR = "TODO CHECK SKIPPED" private val pathToProtoBinary = "scripts/assets/todo_exemptions.pb" private val wikiReferenceNote = "Refer to https://github.com/oppia/oppia-android/wiki/Static-Analysis-Checks" + "#todo-open-checks for more details on how to fix this." + private val regenerateNote = + "There were failures. Re-run //scripts:todo_open_check with \"regenerate\" at the end to " + + "regenerate the exemption file with all failures as exempted." + + @field:[Rule JvmField] val tempFolder = TemporaryFolder() - @Rule - @JvmField - var tempFolder = TemporaryFolder() + private val scriptBgDispatcher by lazy { ScriptBackgroundCoroutineDispatcher() } + private val fakeCommandExecutor by lazy { FakeCommandExecutor() } @Before fun setUp() { tempFolder.newFolder("testfiles") tempFolder.newFolder("scripts", "assets") tempFolder.newFile(pathToProtoBinary) + setUpSupportForGhAuth(TEST_AUTH_TOKEN) System.setOut(PrintStream(outContent)) } @After fun restoreStreams() { System.setOut(originalOut) - } - - @Test - fun testTodoCheck_noJsonFilePresent_checkShouldFail() { - val exception = assertThrows() { runScript() } - - assertThat(exception).hasMessageThat().contains( - "${retrieveTestFilesDirectoryPath()}/open_issues.json: No such file exists" - ) + scriptBgDispatcher.close() } @Test fun testTodoCheck_multipleTodosPresent_allAreValid_checkShouldPass() { - val testJSONContent = - """ - [{"number":11004},{"number":11003},{"number":11002},{"number":11001}] - """.trimIndent() - val testJSONFile = tempFolder.newFile("testfiles/open_issues.json") - testJSONFile.writeText(testJSONContent) + setUpGitHubService(issueNumbers = listOf(11004, 11003, 11002, 11001)) val tempFile1 = tempFolder.newFile("testfiles/TempFile1.kt") val tempFile2 = tempFolder.newFile("testfiles/TempFile2.kt") val testContent1 = @@ -84,12 +86,7 @@ class TodoOpenCheckTest { @Test fun testTodoCheck_onlyPoorlyFormattedTodosPresent_checkShouldFail() { - val testJSONContent = - """ - [] - """.trimIndent() - val testJSONFile = tempFolder.newFile("testfiles/open_issues.json") - testJSONFile.writeText(testJSONContent) + setUpGitHubService(issueNumbers = emptyList()) val tempFile = tempFolder.newFile("testfiles/TempFile.txt") val testContent = """ @@ -107,25 +104,22 @@ class TodoOpenCheckTest { val failureMessage = """ TODOs not in correct format: - - ${retrieveTestFilesDirectoryPath()}/TempFile.txt:1 - - ${retrieveTestFilesDirectoryPath()}/TempFile.txt:2 - - ${retrieveTestFilesDirectoryPath()}/TempFile.txt:3 - - ${retrieveTestFilesDirectoryPath()}/TempFile.txt:4 - - ${retrieveTestFilesDirectoryPath()}/TempFile.txt:5 + - TempFile.txt:1 + - TempFile.txt:2 + - TempFile.txt:3 + - TempFile.txt:4 + - TempFile.txt:5 $wikiReferenceNote + + $regenerateNote """.trimIndent() assertThat(outContent.toString().trim()).isEqualTo(failureMessage) } @Test fun testTodoCheck_onlyOpenIssueFailureTodosPresent_checkShouldFail() { - val testJSONContent = - """ - [{"number":10000000},{"number":100000004}] - """.trimIndent() - val testJSONFile = tempFolder.newFile("testfiles/open_issues.json") - testJSONFile.writeText(testJSONContent) + setUpGitHubService(issueNumbers = listOf(10000000, 100000004)) val tempFile = tempFolder.newFile("testfiles/TempFile.txt") val testContent = """ @@ -143,23 +137,20 @@ class TodoOpenCheckTest { val failureMessage = """ TODOs not corresponding to open issues on GitHub: - - ${retrieveTestFilesDirectoryPath()}/TempFile.txt:1 - - ${retrieveTestFilesDirectoryPath()}/TempFile.txt:2 - - ${retrieveTestFilesDirectoryPath()}/TempFile.txt:5 + - TempFile.txt:1 + - TempFile.txt:2 + - TempFile.txt:5 $wikiReferenceNote + + $regenerateNote """.trimIndent() assertThat(outContent.toString().trim()).isEqualTo(failureMessage) } @Test fun testTodoCheck_multipleFailuresPresent_allFailuresShouldBeReported() { - val testJSONContent = - """ - [{"number":349888},{"number":349777}] - """.trimIndent() - val testJSONFile = tempFolder.newFile("testfiles/open_issues.json") - testJSONFile.writeText(testJSONContent) + setUpGitHubService(issueNumbers = listOf(349888, 349777)) val tempFile1 = tempFolder.newFile("testfiles/TempFile1.kt") val tempFile2 = tempFolder.newFile("testfiles/TempFile2.kt") val testContent1 = @@ -185,26 +176,23 @@ class TodoOpenCheckTest { val failureMessage = """ TODOs not in correct format: - - ${retrieveTestFilesDirectoryPath()}/TempFile1.kt:2 - - ${retrieveTestFilesDirectoryPath()}/TempFile2.kt:1 + - TempFile1.kt:2 + - TempFile2.kt:1 TODOs not corresponding to open issues on GitHub: - - ${retrieveTestFilesDirectoryPath()}/TempFile1.kt:1 - - ${retrieveTestFilesDirectoryPath()}/TempFile2.kt:3 + - TempFile1.kt:1 + - TempFile2.kt:3 $wikiReferenceNote + + $regenerateNote """.trimIndent() assertThat(outContent.toString().trim()).isEqualTo(failureMessage) } @Test fun testTodoCheck_multipleFailuresPresent_loggingShouldBeAsPerLexicographicalOrder() { - val testJSONContent = - """ - [{"number":349888},{"number":349777}] - """.trimIndent() - val testJSONFile = tempFolder.newFile("testfiles/open_issues.json") - testJSONFile.writeText(testJSONContent) + setUpGitHubService(issueNumbers = listOf(349888, 349777)) val tempFile1 = tempFolder.newFile("testfiles/Presenter.kt") val tempFile2 = tempFolder.newFile("testfiles/Fragment.kt") val tempFile3 = tempFolder.newFile("testfiles/Activity.kt") @@ -237,27 +225,24 @@ class TodoOpenCheckTest { val failureMessage = """ TODOs not in correct format: - - ${retrieveTestFilesDirectoryPath()}/Activity.kt:2 - - ${retrieveTestFilesDirectoryPath()}/Fragment.kt:1 - - ${retrieveTestFilesDirectoryPath()}/Presenter.kt:2 + - Activity.kt:2 + - Fragment.kt:1 + - Presenter.kt:2 TODOs not corresponding to open issues on GitHub: - - ${retrieveTestFilesDirectoryPath()}/Fragment.kt:3 - - ${retrieveTestFilesDirectoryPath()}/Presenter.kt:1 + - Fragment.kt:3 + - Presenter.kt:1 $wikiReferenceNote + + $regenerateNote """.trimIndent() assertThat(outContent.toString().trim()).isEqualTo(failureMessage) } @Test fun testTodoCheck_addExemptions_exemptedTodosAreInvalid_checkShouldPass() { - val testJSONContent = - """ - [{"number":11004},{"number":11003},{"number":11002},{"number":11001}] - """.trimIndent() - val testJSONFile = tempFolder.newFile("testfiles/open_issues.json") - testJSONFile.writeText(testJSONContent) + setUpGitHubService(issueNumbers = listOf(11004, 11003, 11002, 11001)) val tempFile1 = tempFolder.newFile("testfiles/TempFile1.kt") val tempFile2 = tempFolder.newFile("testfiles/TempFile2.kt") val testContent1 = @@ -294,12 +279,7 @@ class TodoOpenCheckTest { @Test fun testTodoCheck_allTodosAreValid_redundantExemption_checkShouldFail() { - val testJSONContent = - """ - [{"number":1000000},{"number":152440222},{"number":152440223},{"number":11001}] - """.trimIndent() - val testJSONFile = tempFolder.newFile("testfiles/open_issues.json") - testJSONFile.writeText(testJSONContent) + setUpGitHubService(issueNumbers = listOf(1000000, 152440222, 152440223, 11001)) val tempFile1 = tempFolder.newFile("testfiles/TempFile1.kt") val tempFile2 = tempFolder.newFile("testfiles/TempFile2.kt") val testContent1 = @@ -340,18 +320,15 @@ class TodoOpenCheckTest { - TempFile1.kt:2 - TempFile2.kt:1 Please remove them from scripts/assets/todo_exemptions.textproto + + $regenerateNote """.trimIndent() assertThat(outContent.toString().trim()).isEqualTo(failureMessage) } @Test fun testTodoCheck_combineMultipleFailures_checkShouldFailWithAllErrorsLogged() { - val testJSONContent = - """ - [{"number":1000000},{"number":152440222},{"number":152440223},{"number":11001}] - """.trimIndent() - val testJSONFile = tempFolder.newFile("testfiles/open_issues.json") - testJSONFile.writeText(testJSONContent) + setUpGitHubService(issueNumbers = listOf(1000000, 152440222, 152440223, 11001)) val tempFile1 = tempFolder.newFile("testfiles/TempFile1.kt") val tempFile2 = tempFolder.newFile("testfiles/TempFile2.kt") val testContent1 = @@ -389,27 +366,442 @@ class TodoOpenCheckTest { Please remove them from scripts/assets/todo_exemptions.textproto TODOs not in correct format: - - ${retrieveTestFilesDirectoryPath()}/TempFile2.kt:1 + - TempFile2.kt:1 TODOs not corresponding to open issues on GitHub: - - ${retrieveTestFilesDirectoryPath()}/TempFile1.kt:3 + - TempFile1.kt:3 $wikiReferenceNote + + $regenerateNote """.trimIndent() assertThat(outContent.toString().trim()).isEqualTo(failureMessage) } - /** Retrieves the absolute path of testfiles directory. */ - private fun retrieveTestFilesDirectoryPath(): String { - return "${tempFolder.root}/testfiles" + @Test + fun testRegenerate_noTodos_checkShouldSkip() { + setUpGitHubService(issueNumbers = listOf(11004, 11003, 11002, 11001)) + val tempFile1 = tempFolder.newFile("testfiles/TempFile1.kt") + val tempFile2 = tempFolder.newFile("testfiles/TempFile2.kt") + val testContent1 = + """ + test Todo + test TODO + """.trimIndent() + val testContent2 = + """ + todo + """.trimIndent() + tempFile1.writeText(testContent1) + tempFile2.writeText(testContent2) + + val exception = assertThrows() { runScript(regenerateFile = true) } + + // 'regenerate' always throws an exception since it's regenerating everything. + assertThat(exception).hasMessageThat().contains(TODO_SYNTAX_CHECK_SKIPPED_OUTPUT_INDICATOR) + } + + @Test + fun testRegenerate_onlyValidTodos_checkShouldSkip() { + setUpGitHubService(issueNumbers = listOf(11004, 11003, 11002, 11001)) + val tempFile1 = tempFolder.newFile("testfiles/TempFile1.kt") + val tempFile2 = tempFolder.newFile("testfiles/TempFile2.kt") + val testContent1 = + """ + // TODO(#11002): test summary 1. + # TODO(#11004): test summary 2. + test Todo + test TODO + """.trimIndent() + val testContent2 = + """ + // TODO(#11001): test summary 3. + todo + + + """.trimIndent() + tempFile1.writeText(testContent1) + tempFile2.writeText(testContent2) + + val exception = assertThrows() { runScript(regenerateFile = true) } + + // 'regenerate' always throws an exception since it's regenerating everything. + assertThat(exception).hasMessageThat().contains(TODO_SYNTAX_CHECK_SKIPPED_OUTPUT_INDICATOR) + } + + @Test + fun testRegenerate_poorlyFormattedTodos_checkShouldSkip() { + setUpGitHubService(issueNumbers = emptyList()) + val tempFile = tempFolder.newFile("testfiles/TempFile.txt") + val testContent = + """ + // TODO (#1044): test + # TODO(102) + + // some test conent TODO(#1020000): test description. + some test content TODO(#100002): some description. + """.trimIndent() + tempFile.writeText(testContent) + + val exception = assertThrows() { runScript(regenerateFile = true) } + + assertThat(exception).hasMessageThat().contains(TODO_SYNTAX_CHECK_SKIPPED_OUTPUT_INDICATOR) + } + + @Test + fun testRegenerate_openIssuesTodos_checkShouldSkip() { + setUpGitHubService(issueNumbers = listOf(10000000, 100000004)) + val tempFile = tempFolder.newFile("testfiles/TempFile.txt") + val testContent = + """ + // TODO(#104444444): test summary 1. + # TODO(#10210110): test summary 2. + test todo + some test content Todo + + """.trimIndent() + tempFile.writeText(testContent) + + val exception = assertThrows() { runScript(regenerateFile = true) } + + assertThat(exception).hasMessageThat().contains(TODO_SYNTAX_CHECK_SKIPPED_OUTPUT_INDICATOR) + } + + @Test + fun testRegenerate_mixedBadFormattingAndOpenAndValidTodos_checkShouldSkip() { + setUpGitHubService(issueNumbers = listOf(10000000, 100000004, 11002, 11004)) + val tempFile = tempFolder.newFile("testfiles/TempFile.txt") + val testContent = + """ + // TODO (#1044): test + # TODO(102) + + // some test conent TODO(#1020000): test description. + some test content TODO(#100002): some description. + // TODO(#104444444): test summary 1. + # TODO(#10210110): test summary 2. + test todo + some test content Todo + + // TODO(#11002): test summary 1. + # TODO(#11004): test summary 2. + test Todo + test TODO + """.trimIndent() + tempFile.writeText(testContent) + + val exception = assertThrows() { runScript(regenerateFile = true) } + + assertThat(exception).hasMessageThat().contains(TODO_SYNTAX_CHECK_SKIPPED_OUTPUT_INDICATOR) + } + + @Test + fun testRegenerate_todosWithExemptions_checkShouldSkip() { + setUpGitHubService(issueNumbers = listOf(11004, 11003, 11002, 11001)) + val tempFile1 = tempFolder.newFile("testfiles/TempFile1.kt") + val tempFile2 = tempFolder.newFile("testfiles/TempFile2.kt") + val testContent1 = + """ + // TODO (152440222): test description 1 + """.trimIndent() + val testContent2 = + """ + # TODO(#1000000): test description 2 + """.trimIndent() + tempFile1.writeText(testContent1) + tempFile2.writeText(testContent2) + val exemptionFile = File("${tempFolder.root}/$pathToProtoBinary") + val exemptions = TodoOpenExemptions.newBuilder().apply { + this.addAllTodoOpenExemption( + listOf( + TodoOpenExemption.newBuilder().apply { + this.exemptedFilePath = "TempFile1.kt" + this.addAllLineNumber(listOf(1)).build() + }.build(), + TodoOpenExemption.newBuilder().apply { + this.exemptedFilePath = "TempFile2.kt" + this.addAllLineNumber(listOf(1)).build() + }.build() + ) + ) + }.build() + exemptions.writeTo(exemptionFile.outputStream()) + + val exception = assertThrows() { runScript(regenerateFile = true) } + + // 'regenerate' always throws an exception since it's regenerating everything. + assertThat(exception).hasMessageThat().contains(TODO_SYNTAX_CHECK_SKIPPED_OUTPUT_INDICATOR) + } + + @Test + fun testRegenerate_noTodos_shouldOutputEmptyTextProto() { + setUpGitHubService(issueNumbers = listOf(11004, 11003, 11002, 11001)) + val tempFile1 = tempFolder.newFile("testfiles/TempFile1.kt") + val tempFile2 = tempFolder.newFile("testfiles/TempFile2.kt") + val testContent1 = + """ + test Todo + test TODO + """.trimIndent() + val testContent2 = + """ + todo + """.trimIndent() + tempFile1.writeText(testContent1) + tempFile2.writeText(testContent2) + + assertThrows() { runScript(regenerateFile = true) } + + val failureMessage = + """ + Regenerated exemptions: + """.trimIndent() + assertThat(outContent.toString().trim()).isEqualTo(failureMessage) + } + + @Test + fun testRegenerate_onlyValidTodos_shouldOutputEmptyTextProto() { + setUpGitHubService(issueNumbers = listOf(11004, 11003, 11002, 11001)) + val tempFile1 = tempFolder.newFile("testfiles/TempFile1.kt") + val tempFile2 = tempFolder.newFile("testfiles/TempFile2.kt") + val testContent1 = + """ + // TODO(#11002): test summary 1. + # TODO(#11004): test summary 2. + test Todo + test TODO + """.trimIndent() + val testContent2 = + """ + // TODO(#11001): test summary 3. + todo + + + """.trimIndent() + tempFile1.writeText(testContent1) + tempFile2.writeText(testContent2) + + assertThrows() { runScript(regenerateFile = true) } + + val failureMessage = + """ + Regenerated exemptions: + """.trimIndent() + assertThat(outContent.toString().trim()).isEqualTo(failureMessage) + } + + @Test + fun testRegenerate_poorlyFormattedTodos_shouldOutputNewTextProto() { + setUpGitHubService(issueNumbers = emptyList()) + val tempFile = tempFolder.newFile("testfiles/TempFile.txt") + val testContent = + """ + // TODO (#1044): test + # TODO(102) + + // some test conent TODO(#1020000): test description. + some test content TODO(#100002): some description. + """.trimIndent() + tempFile.writeText(testContent) + + assertThrows() { runScript(regenerateFile = true) } + + val failureMessage = + """ + TODOs not in correct format: + - TempFile.txt:1 + - TempFile.txt:2 + - TempFile.txt:3 + - TempFile.txt:4 + - TempFile.txt:5 + + $wikiReferenceNote + + Regenerated exemptions: + + todo_open_exemption { + exempted_file_path: "TempFile.txt" + line_number: 1 + line_number: 2 + line_number: 3 + line_number: 4 + line_number: 5 + } + """.trimIndent() + assertThat(outContent.toString().trim()).isEqualTo(failureMessage) + } + + @Test + fun testRegenerate_openIssuesTodos_shouldOutputNewTextProto() { + setUpGitHubService(issueNumbers = listOf(10000000, 100000004)) + val tempFile = tempFolder.newFile("testfiles/TempFile.txt") + val testContent = + """ + // TODO(#104444444): test summary 1. + # TODO(#10210110): test summary 2. + test todo + some test content Todo + + """.trimIndent() + tempFile.writeText(testContent) + + assertThrows() { runScript(regenerateFile = true) } + + val failureMessage = + """ + TODOs not corresponding to open issues on GitHub: + - TempFile.txt:1 + - TempFile.txt:2 + - TempFile.txt:5 + + $wikiReferenceNote + + Regenerated exemptions: + + todo_open_exemption { + exempted_file_path: "TempFile.txt" + line_number: 1 + line_number: 2 + line_number: 5 + } + """.trimIndent() + assertThat(outContent.toString().trim()).isEqualTo(failureMessage) + } + + @Test + fun testRegenerate_mixedBadFormattingAndOpenAndValidTodos_shouldOutputNewTextProto() { + setUpGitHubService(issueNumbers = listOf(10000000, 100000004, 11002, 11004)) + val tempFile = tempFolder.newFile("testfiles/TempFile.txt") + val testContent = + """ + // TODO (#1044): test + # TODO(102) + + // some test conent TODO(#1020000): test description. + some test content TODO(#100002): some description. + // TODO(#104444444): test summary 1. + # TODO(#10210110): test summary 2. + test todo + some test content Todo + + // TODO(#11002): test summary 1. + # TODO(#11004): test summary 2. + test Todo + test TODO + """.trimIndent() + tempFile.writeText(testContent) + + assertThrows() { runScript(regenerateFile = true) } + + val failureMessage = + """ + TODOs not in correct format: + - TempFile.txt:1 + - TempFile.txt:2 + - TempFile.txt:3 + - TempFile.txt:4 + - TempFile.txt:5 + + TODOs not corresponding to open issues on GitHub: + - TempFile.txt:6 + - TempFile.txt:7 + - TempFile.txt:10 + + $wikiReferenceNote + + Regenerated exemptions: + + todo_open_exemption { + exempted_file_path: "TempFile.txt" + line_number: 1 + line_number: 2 + line_number: 3 + line_number: 4 + line_number: 5 + line_number: 6 + line_number: 7 + line_number: 10 + } + """.trimIndent() + assertThat(outContent.toString().trim()).isEqualTo(failureMessage) + } + + @Test + fun testRegenerate_todosWithExemptions_shouldOutputNewTextProtoIncludingExemptions() { + setUpGitHubService(issueNumbers = listOf(11004, 11003, 11002, 11001)) + val tempFile1 = tempFolder.newFile("testfiles/TempFile1.kt") + val tempFile2 = tempFolder.newFile("testfiles/TempFile2.kt") + val testContent1 = + """ + // TODO (152440222): test description 1 + """.trimIndent() + val testContent2 = + """ + # TODO(#1000000): test description 2 + """.trimIndent() + tempFile1.writeText(testContent1) + tempFile2.writeText(testContent2) + val exemptionFile = File("${tempFolder.root}/$pathToProtoBinary") + val exemptions = TodoOpenExemptions.newBuilder().apply { + this.addAllTodoOpenExemption( + listOf( + TodoOpenExemption.newBuilder().apply { + this.exemptedFilePath = "TempFile1.kt" + this.addAllLineNumber(listOf(1)).build() + }.build(), + TodoOpenExemption.newBuilder().apply { + this.exemptedFilePath = "TempFile2.kt" + this.addAllLineNumber(listOf(1)).build() + }.build() + ) + ) + }.build() + exemptions.writeTo(exemptionFile.outputStream()) + + assertThrows() { runScript(regenerateFile = true) } + + val failureMessage = + """ + Regenerated exemptions: + + todo_open_exemption { + exempted_file_path: "TempFile1.kt" + line_number: 1 + } + todo_open_exemption { + exempted_file_path: "TempFile2.kt" + line_number: 1 + } + """.trimIndent() + assertThat(outContent.toString().trim()).isEqualTo(failureMessage) + } + + private fun setUpGitHubService(issueNumbers: List) { + val issueJsons = issueNumbers.joinToString(separator = ",") { "{\"number\":$it}" } + val mockWebServer = MockWebServer() + mockWebServer.enqueue(MockResponse().setBody("[$issueJsons]")) + mockWebServer.enqueue(MockResponse().setBody("[]")) // No more issues. + GitHubClient.remoteApiUrl = mockWebServer.url("/").toString() + } + + private fun setUpSupportForGhAuth(authToken: String) { + fakeCommandExecutor.registerHandler("gh") { _, args, outputStream, _ -> + when (args) { + listOf("help") -> 0 + listOf("auth", "token") -> 0.also { outputStream.print(authToken) } + else -> 1 + } + } } - /** Runs the todo_open_check. */ - private fun runScript() { - main( - retrieveTestFilesDirectoryPath(), - "${tempFolder.root}/$pathToProtoBinary", - "open_issues.json" + // TODO(#5314): Replace this (& other script tests) with using main() directly and swap out + // dependencies using Dagger rather than needing to call into a separately created instance of an + // internal helper class for the script. + private fun runScript(regenerateFile: Boolean = false) { + val repoRoot = File(tempFolder.root, "testfiles") + TodoOpenCheck(repoRoot, scriptBgDispatcher, fakeCommandExecutor).runTodoOpenCheck( + pathToProtoBinary = "${tempFolder.root}/$pathToProtoBinary", + regenerateFile ) } } diff --git a/scripts/static_checks.sh b/scripts/static_checks.sh index 688e542602e..f7ad2a39d8e 100644 --- a/scripts/static_checks.sh +++ b/scripts/static_checks.sh @@ -105,4 +105,11 @@ echo "********************************" echo "Running license texts checks" echo "********************************" bazel run //scripts:license_texts_check -- $(pwd)/app/src/main/res/values/third_party_dependencies.xml +echo "" +# TODO checks. +echo "********************************" +echo "Running TODO correctness checks" +echo "********************************" +bazel run //scripts:todo_open_check -- $(pwd) scripts/assets/todo_open_exemptions.pb +echo "" diff --git a/wiki/Static-Analysis-Checks.md b/wiki/Static-Analysis-Checks.md index e4252c7c445..372a9b27bc1 100644 --- a/wiki/Static-Analysis-Checks.md +++ b/wiki/Static-Analysis-Checks.md @@ -71,7 +71,7 @@ In general, failures for this check should be fixed by moving the file to the co filename_checks { prohibited_filename_regex: "^((?!(app|testing)).)+/src/main/.+?Activity.kt" failure_message: "Activities cannot be placed outside the app or testing module" - exempted_file_name: "testing/src/main/SampleActivity.kt" + exempted_file_name: "testing/src/main/SampleActivity.kt" } ``` 2. Add an explanation to your PR description detailing why this exemption is correct @@ -174,6 +174,22 @@ This check ensures that every TODO present in the codebase corresponds to open i ### Purpose To avoid scenarios where a TODO was added not corresponding to an open issue on GitHub, this check is particularly needed. Having a corresponding issue for a TODO helps us to track its progress. Also, in order to maintain consistency we want all the TODOs to be formatted as per the convention. +### Running the command +The TODO open check can be run as follows: +```sh +bazel run //scripts:todo_open_check -- $(pwd) scripts/assets/todo_open_exemptions.pb +``` + +Please note, however, that this tool requires the GitHub CLI to be set up and _logged in_ on your local machine. Specifically, you'll need to: +- Follow the instructions on the GitHub CLI repository page to install the tool: https://github.com/cli/cli?tab=readme-ov-file#installation. +- Follow the instructions on the tool's official documentation to authenticate (e.g. via ``gh auth login``): https://cli.github.com/manual/gh_auth_login. + +Some things to note: +- If you're using a headless environment, you can force the browser check to fail by setting the ``GH_BROWSER`` variable to a non-existent command ahead of ``gh auth login``. This allows you to authenticate on a different machine. +- You don't need to authenticate using a browser. This defaults to creating a PAT (personal access token) with full access to your account. While that can be helpful, it may not be what you want (in which case you can create a fine-grained PAT for authenticating the tool). The minimum required scopes for the token are mentioned in the command's output while authenticating the tool. + +If you decide to use a personal access token, you can generate a fine-grained one via the Developer settings section of your GitHub profile. It only needs access to public repositories and no other permissions for the purpose of Oppia's scripts. + ### Fixing failures #### TODO formatting failure @@ -196,6 +212,7 @@ To fix this failure: there are 3 ways: 1. Repurpose the TODO to a new issue. 2. If the TODO has been resolved then please remove it from the repository. 3. Reopen the issue. + #### Case when using ‘TODO’ keyword for documentation purposes If it’s a case where a ‘TODO’ keyword has been used for documentation purposes or if it's not meant to correspond to a future work, then please add an exemption for it. Add a new TODO exemption in the [scripts/assets/todo_open_exemptions.textproto](https://github.com/oppia/oppia-android/blob/2da95a53928bc989f5959fbac211f7f7ca0a753f/scripts/assets/todo_open_exemptions.textproto). Example: @@ -206,6 +223,12 @@ todo_open_exemption { } ``` +Alternatively, the textproto can be entirely generated by passing ``regenerate`` to the command as so: + +```sh +bazel run //scripts:todo_open_check -- $(pwd) scripts/assets/todo_open_exemptions.pb regenerate +``` + ## TODO issue resolved check The check ensures that a TODO issue is not closed until all of its corresponding TODO items are resolved.