Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #5431 : Todo Checks Should Check Exclusively Against Issues #5564

Merged
merged 31 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
d819e70
Merge pull request #1 from oppia/develop
subhajitxyz Jul 23, 2024
c6f54ea
Merge branch 'oppia:develop' into develop
subhajitxyz Jul 27, 2024
d44015b
Merge remote-tracking branch 'upstream/develop' into develop
subhajitxyz Aug 17, 2024
d604bc2
Merge pull request #2 from oppia/develop
subhajitxyz Aug 26, 2024
cc0ddef
Merge pull request #3 from oppia/develop
subhajitxyz Aug 26, 2024
52c6bb1
Merge pull request #4 from oppia/develop
subhajitxyz Aug 29, 2024
773c810
Merge remote-tracking branch 'upstream/develop' into develop
subhajitxyz Sep 5, 2024
56af5ae
Merge pull request #5 from oppia/develop
subhajitxyz Sep 16, 2024
3883f70
Merge pull request #6 from oppia/develop
subhajitxyz Sep 27, 2024
10c8e6e
Fix #5404: Migrate away from onBackPressed for remaining activities (…
dattasneha Oct 3, 2024
5e140e9
Fix #5404: Migrate away from onBackPressed for RevisionCardActivity (…
dattasneha Oct 9, 2024
b4ad7a3
Merge branch 'oppia:develop' into develop
subhajitxyz Oct 11, 2024
238645d
Merge pull request #8 from oppia/develop
subhajitxyz Oct 12, 2024
5a546b9
Merge pull request #9 from oppia/develop
subhajitxyz Nov 3, 2024
63d5e5c
add pull=false
subhajitxyz Nov 3, 2024
5806dc7
add pr in todo for testing
subhajitxyz Nov 3, 2024
c2862f3
correct the existing todo
subhajitxyz Nov 3, 2024
594459c
add test for check pr
subhajitxyz Nov 7, 2024
7b3daf3
hi
subhajitxyz Nov 8, 2024
040f7cc
add pull false
subhajitxyz Nov 11, 2024
a43c39f
add test
subhajitxyz Nov 11, 2024
e01392b
correct klint
subhajitxyz Nov 11, 2024
5618c23
remove false from fetchOpenIssue
subhajitxyz Nov 11, 2024
f5b6e61
modify githubissue model
subhajitxyz Nov 11, 2024
42fccfb
solved klint
subhajitxyz Nov 11, 2024
2bd406a
fix exemptions
subhajitxyz Nov 11, 2024
277fabb
add lines in exemptions
subhajitxyz Nov 11, 2024
2c505b0
correct Kdoc and test
subhajitxyz Nov 11, 2024
4c93f26
Merge branch 'develop' into fix-todo-checks
subhajitxyz Nov 11, 2024
e8e2212
remove unnessery comments
subhajitxyz Nov 11, 2024
0b8ef62
Merge branch 'develop' into fix-todo-checks
subhajitxyz Nov 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions scripts/assets/todo_open_exemptions.textproto
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,11 @@ todo_open_exemption {
line_number: 689
line_number: 737
line_number: 741
line_number: 790
line_number: 791
line_number: 792
line_number: 798
line_number: 800
}
todo_open_exemption {
exempted_file_path: "scripts/static_checks.sh"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,14 @@ class GitHubClient(
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."
}
}.filter { it.pullRequest == null }
adhiamboperes marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,18 @@ import com.squareup.moshi.JsonClass
* 'issues/' in an issue's GitHub URL)
*/
@JsonClass(generateAdapter = true)
data class GitHubIssue(@Json(name = "number") val number: Int)
data class GitHubIssue(
@Json(name = "number") val number: Int,
@Json(name = "pull_request") val pullRequest: PullRequestInfo? = null
)

/**
* Data class representing information about a pull request associated with a GitHub issue.
*
* @property url the URL of the pull request, if it exists. This provides the link to the specific
* pull request associated with the issue on GitHub.
*/
@JsonClass(generateAdapter = true)
data class PullRequestInfo(
@Json(name = "url") val url: String? = null
)
Original file line number Diff line number Diff line change
Expand Up @@ -777,11 +777,67 @@ class TodoOpenCheckTest {
assertThat(outContent.toString().trim()).isEqualTo(failureMessage)
}

private fun setUpGitHubService(issueNumbers: List<Int>) {
val issueJsons = issueNumbers.joinToString(separator = ",") { "{\"number\":$it}" }
@Test
fun testTodoCheck_PrPresent_checkShouldFail() {
setUpGitHubService(
issueNumbers = listOf(11004, 11003, 11002, 11001),
pullRequestNumbers = listOf(11005)
)
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.
# TODO(#11001): test summary 3.
test Todo
test TODO
""".trimIndent()
val testContent2 =
"""
// TODO(#11005): test summary 3.
todo
<!-- TODO(#11003): test summary 4-->

""".trimIndent()
tempFile1.writeText(testContent1)
tempFile2.writeText(testContent2)

val exception = assertThrows<Exception>() { runScript() }

assertThat(exception).hasMessageThat().contains(TODO_SYNTAX_CHECK_FAILED_OUTPUT_INDICATOR)
val failureMessage =
"""
TODOs not corresponding to open issues on GitHub:
- TempFile2.kt:1

$wikiReferenceNote

$regenerateNote
""".trimIndent()
assertThat(outContent.toString().trim()).isEqualTo(failureMessage)
}

private fun setUpGitHubService(
issueNumbers: List<Int>,
pullRequestNumbers: List<Int> = emptyList()
) {
// Create JSON for issues with "pull_request" set to null
val issueJsons = issueNumbers
.joinToString(separator = ",") { "{\"number\":$it,\"pull_request\":null}" }

// Create JSON for pull requests with "pull_request" as an empty object
val pullRequestJsons = pullRequestNumbers
.joinToString(separator = ",") { "{\"number\":$it,\"pull_request\":{}}" }

// Combine issues and pull requests into one JSON array
val combinedJsons =
"[$issueJsons${if (pullRequestNumbers.isNotEmpty()) ", $pullRequestJsons" else ""}]"

val mockWebServer = MockWebServer()
mockWebServer.enqueue(MockResponse().setBody("[$issueJsons]"))
mockWebServer.enqueue(MockResponse().setBody("[]")) // No more issues.
mockWebServer.enqueue(MockResponse().setBody(combinedJsons))
mockWebServer.enqueue(MockResponse().setBody("[]"))
GitHubClient.remoteApiUrl = mockWebServer.url("/").toString()
}

Expand Down
Loading