Compute affected tests in CI workflow is omitting targets affected by committed renamed files

[BenHenning]

See #4020 (comment) for specifics. However, because the script is computing the wrong base branch, in some cases it may miss changes (such as was noticed in #4020) which is a serious problem since we won't end up running affected targets until after the PR is merged.

#4034 is my working PR to investigate.

[BenHenning]

Follow-up: I thought the issue was because the merge-base was different between the branch & running it locally (I think this is because GitHub actually generates a merge commit which seems to combine the latest develop with the changes in the PR branch & then runs CI workflows against that merge commit).

However, forcing CI workflow to use the HEAD commit for the PR didn't fix the issue (per https://github.com/actions/checkout#checkout-pull-request-head-commit-instead-of-merge-commit).

Upon further investigation, I realized that there's a difference in Git versions. My local version is 2.7.4 where the script behaves correctly, and CI is using Git 2.34.1. Since GitHub updates our environment from underneath us, this is an undiscovered regression. Interestingly, the compute affected tests test suite didn't catch this despite using real Git (though the root cause is still unknown since it's not clear what the actual regression is yet). My guess is that a relatively new version of Git changed how it treats moved files reported in git diff.

[BenHenning]

https://stackoverflow.com/a/10121060/3689782 provides some context here. git diff on the old version would output both the old & new files, whereas with -M (& with the new git without any parameter) it only outputs the old files. Because we use this list to compute affected targets, the result is wrong. I'm trying to figure out how to get git diff to provide the correct file names. We'll also need a test that specifically verifies a committed moved file.

[BenHenning]

git 2.9.0 was the version when rename detection was automatically enabled upon git diff (see https://github.com/git/git/blob/142430338477d9d1bb25be66267225fb58498d92/Documentation/RelNotes/2.9.0.txt#L33) so this issue has existed since the script was introduced in CI.

[BenHenning]

https://stackoverflow.com/a/55839215/3689782 is a help, and I realize that all we need to add is --no-renames to disable the renaming detection (& thus move back to the old git behavior). Gonna need to add a test to ensure that this can actually be caught it if were to ever regress since it's a rather serious failure in the script.

[Rd4dev]

Following up on #3022 (comment)

You made some changes to BazelClient--do you think that change would fix ComputeAffectedTests for moved files, as well?

@BenHenning, with the case of ComputeChangedFiles I did not modify any BazelClient stuff rather I used GitClient to retrieve the renamed / moved files using the following:

private fun retrieveRenamedFiles(): List<String> {
  val renamedFilesCommand = executeGitCommand("diff -M --name-status ${computeCommitRange()}")
  return renamedFilesCommand.filter { it.startsWith("R") }
    .map { it.split("\t")[1] }
}

The command execution:

$ git diff -M --name-status HEAD..3b7ddb91bf67fcbc9abace491f680fdfcd2aa538
M       .github/workflows/unit_tests.yml
M       scripts/src/java/org/oppia/android/scripts/common/GitClient.kt
R100    scripts/src/java/org/oppia/android/scripts/coverage/CoverageReporter.kt scripts/src/java/org/oppia/android/scripts/coverage/reporter/CoverageReporter.kt
M       utility/src/main/java/org/oppia/android/util/math/MathTokenizer.kt

Would this be something that can be done with ComputAffectedTests to include targets affected by renamed files or is that different in this case?

I tried implementing the same for ComputeAffectedTests by just including the renamedFiles lists to changedFiles,

Changed Files Lists & Stack Traces

Initial Changed Files List without moving files and adding the retrieval of renamed files method -
Changed files (per Git, 2 total): stack trace

[.github/workflows/unit_tests.yml, utility/src/main/java/org/oppia/android/util/math/MathTokenizer.kt].

After moving CoverageReporter.kt from coverage/reporter -> coverage:
Without retrieval of renamed files -
Changed files (per Git, 2 total): stack trace

[.github/workflows/unit_tests.yml, utility/src/main/java/org/oppia/android/util/math/MathTokenizer.kt].

After moving CoverageReporter.kt from coverage/reporter -> coverage:
With retrieval of renamed files included for retrieveChangedFilesWithPotentialDuplicates() -
Changed files (per Git, 4 total): stack trace

[.github/workflows/unit_tests.yml, scripts/src/java/org/oppia/android/scripts/common/GitClient.kt, utility/src/main/java/org/oppia/android/util/math/MathTokenizer.kt, scripts/src/java/org/oppia/android/scripts/coverage/CoverageReporter.kt].

Seems to provide the newly moved filenames as the result, and could that also help with this - #4035 (comment)