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.