Skip to content

Commit

Permalink
Fix #3292: Add check for test files presence for prod files (#3343)
Browse files Browse the repository at this point in the history
* Fix #3290: Add support for generic regex pattern matching

* Apply review suggestions for proto location

* Delete main directory

* Implment script logic and tests

* CI setup part 1

* syntax fix in yaml file

* import nits in dummy file

* Use regex patterns from script in script test

* Make PR suggestions into effect

* Make PR suggestions into effect

* Make PR suggestions into effect

* Improve naming of script_assets variables

* Make PR suggestions into effect

* Make PR suggestions

* Make PR suggestions

* Revamp testing approach

* Fix #3291: Add check for XML syntax correctness

* nit fixes

* Introduce XML syntax check in static checks workflow

* Fix #3292: Add check for test files presence for prod files

* Make nit suggestions

* Make nit suggestions

* Make nit suggestions

* Add ScriptConstants file to exemptions test list

* Add if: always()

* Add if: always()

* Apply review suggestions on PR

* Make review suggestions from regex pattern checks

* Implement review suggestions based on #3340

* Refactor PR as per feedback recieved

* Implement feedback suggestions

* Implement review suggestions

* Implement review suggestions

* Implement review suggestions

* Implement review suggestions

* nits

* Nit fixes

* Nit fixes

* Nit fixes

* nit fix

* nit fix

* Review suggestions part 1

* Implement review suggestions part 2

* nit fixes

* update static_checks

* add test to repository file

* nit fix

* nit fix

* nit fix

* Do review suggestions

* Implement review suggestions

* Implement review suggestions

* nit fix

* Implement review suggestions

* Add Ben as a codeowner for the ScriptExemptions file

* bazel files nit fix

* Introduce a library for the regex assets

* Make directory structure same as that of #3374

* Make directory structure similar to #3374

* add new files to test file script exemptions and nit fixes

* diable ktlint max-line-length

* disable ktlint-max-line

* disable ktlint max-length

* remove testonly attribute from tests

* add new files to script exemptions

* nit fix

* nit fix

* Apply review suggestions

* ktlint fix

* nit fix

* Apply nit fixes

* Apply nit fixes

* Remove script constants

* nit fix

* add todo

* add todo

* nit fixes

* add test

* add testOnly

* nit fixes

* nit fix

* nit fixes

* nit fixes

* nit fixes

* nit fixes

* fix textproto file

* add exemptions to textproto

* sort failures lexicographically

* add new files to exemption list
  • Loading branch information
Sparsh1212 authored Jul 9, 2021
1 parent 96e6131 commit 9abd017
Show file tree
Hide file tree
Showing 10 changed files with 1,014 additions and 1 deletion.
5 changes: 5 additions & 0 deletions .github/workflows/static_checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -113,3 +113,8 @@ jobs:
if: always()
run: |
bazel run //scripts:xml_syntax_check -- $(pwd)
- name: Testfile Presence Check
if: always()
run: |
bazel run //scripts:test_file_check -- $(pwd)
26 changes: 25 additions & 1 deletion scripts/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@ Kotlin-based scripts to help developers or perform continuous integration tasks.
"""

load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_jvm_binary", "kt_jvm_library")
load("//scripts:script_assets.bzl", "generate_regex_assets_list_from_text_protos")
load(
"//scripts:script_assets.bzl",
"generate_regex_assets_list_from_text_protos",
"generate_test_file_assets_list_from_text_protos",
)

# Visibility for libraries that should be accessible to script tests.
package_group(
Expand Down Expand Up @@ -74,3 +78,23 @@ kt_jvm_binary(
main_class = "org.oppia.android.scripts.xml.XmlSyntaxCheckKt",
runtime_deps = ["//scripts/src/java/org/oppia/android/scripts/xml:xml_syntax_check_lib"],
)

TEST_FILE_EXEMPTION_ASSETS = generate_test_file_assets_list_from_text_protos(
name = "test_file_exemption_assets",
test_file_exemptions_name = ["test_file_exemptions"],
)

kt_jvm_library(
name = "test_file_check_assets",
testonly = True,
data = TEST_FILE_EXEMPTION_ASSETS,
visibility = [":oppia_script_test_visibility"],
)

kt_jvm_binary(
name = "test_file_check",
testonly = True,
data = TEST_FILE_EXEMPTION_ASSETS,
main_class = "org.oppia.android.scripts.testfile.TestFileCheckKt",
runtime_deps = ["//scripts/src/java/org/oppia/android/scripts/testfile:test_file_check_lib"],
)
629 changes: 629 additions & 0 deletions scripts/assets/test_file_exemptions.textproto

Large diffs are not rendered by default.

24 changes: 24 additions & 0 deletions scripts/script_assets.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,27 @@ def generate_regex_assets_list_from_text_protos(
proto_dep_bazel_target_prefix = "//scripts/src/java/org/oppia/android/scripts/proto",
proto_package = "proto",
)

def generate_test_file_assets_list_from_text_protos(
name,
test_file_exemptions_name):
"""
Converts multiple lists of text proto assets to binary.
Args:
name: str. The name of this generation instance. This will be a prefix for derived targets.
test_file_exemptions_name: list of str. The list of test file exemptions file names.
Returns:
list of str. The list of new proto binary asset files that were generated.
"""
return generate_proto_binary_assets(
name = name,
names = test_file_exemptions_name,
proto_dep_name = "test_file_exemptions",
proto_type_name = "TestFileExemptions",
name_prefix = name,
asset_dir = "assets",
proto_dep_bazel_target_prefix = "//scripts/src/java/org/oppia/android/scripts/proto",
proto_package = "proto",
)
12 changes: 12 additions & 0 deletions scripts/src/java/org/oppia/android/scripts/proto/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,15 @@ java_lite_proto_library(
visibility = ["//scripts:oppia_script_library_visibility"],
deps = [":file_content_validation_checks_proto"],
)

proto_library(
name = "test_file_exemptions_proto",
srcs = ["script_exemptions.proto"],
visibility = ["//scripts:oppia_script_binary_visibility"],
)

java_lite_proto_library(
name = "test_file_exemptions_java_proto_lite",
visibility = ["//scripts:oppia_script_library_visibility"],
deps = [":test_file_exemptions_proto"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
syntax = "proto3";

package proto;

option java_package = "org.oppia.android.scripts.proto";
option java_multiple_files = true;

// Exemptions for the test-file check. Exemptions indicate files for which we should not check for
// having test files.
message TestFileExemptions {
// List of all the files which should be exempted for the test-file check. For exempting any
// file, provide its relative path to root in 'script/assets/test_file_exemptions.textproto'.
// Also, note that the file paths in the textproto file are maintained in lexicographical order.
// While adding any new file, please add it only at the correct lexicographical position, so that
// the list remains sorted.
//
// For example if we want to add the 'ActivityComponent.kt' file to the exemption list, add:
// 'exempted_file_path: "app/src/main/java/org/oppia/android/app/activity/ActivityComponent.kt"'
// at the correct lexicographical position in the textproto file.
repeated string exempted_file_path = 1;
}
17 changes: 17 additions & 0 deletions scripts/src/java/org/oppia/android/scripts/testfile/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
"""
Libraries corresponding to test file-related checks, such as ensuring that all the production
(all Kotlin files) files have a corresponding test file present.
"""

load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_jvm_library")

kt_jvm_library(
name = "test_file_check_lib",
testonly = True,
srcs = ["TestFileCheck.kt"],
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:test_file_exemptions_java_proto_lite",
],
)
108 changes: 108 additions & 0 deletions scripts/src/java/org/oppia/android/scripts/testfile/TestFileCheck.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
package org.oppia.android.scripts.testfile

import org.oppia.android.scripts.common.RepositoryFile
import org.oppia.android.scripts.proto.TestFileExemptions
import java.io.File
import java.io.FileInputStream

/**
* Script for ensuring that all production files have test files present.
*
* Usage:
* bazel run //scripts:test_file_check -- <path_to_directory_root>
*
* Arguments:
* - path_to_directory_root: directory path to the root of the Oppia Android repository.
*
* Example:
* bazel run //scripts:test_file_check -- $(pwd)
*/
fun main(vararg args: String) {
// Path of the repo to be analyzed.
val repoPath = "${args[0]}/"

val testFileExemptiontextProto = "scripts/assets/test_file_exemptions"

// A list of all the files to be exempted for this check.
// TODO(#3436): Develop a mechanism for permanently exempting files which do not ever need tests.
val testFileExemptionList = loadTestFileExemptionsProto(testFileExemptiontextProto)
.getExemptedFilePathList()

// A list of all kotlin files in the repo to be analyzed.
val searchFiles = RepositoryFile.collectSearchFiles(
repoPath = repoPath,
expectedExtension = ".kt",
exemptionsList = testFileExemptionList
)

// A list of all the prod files present in the repo.
val prodFilesList = searchFiles.filter { file -> !file.name.endsWith("Test.kt") }

// A list of all the test files present in the repo.
val testFilesList = searchFiles.filter { file -> file.name.endsWith("Test.kt") }

// A list of all the prod files that do not have a corresponding test file.
val matchedFiles = prodFilesList.filter { prodFile ->
!testFilesList.any { testFile ->
testFile.name == computeExpectedTestFileName(prodFile)
}
}

logFailures(matchedFiles, testFileExemptiontextProto)

if (matchedFiles.isNotEmpty()) {
throw Exception("TEST FILE CHECK FAILED")
} else {
println("TEST FILE CHECK PASSED")
}
}

/**
* Computes the expected test file name for a prod file.
*
* @param prodFile the prod file for which expected test file name has to be computed
* @return expected name of the test file
*/
private fun computeExpectedTestFileName(prodFile: File): String {
return "${prodFile.nameWithoutExtension}Test.kt"
}

/**
* Logs the file names of all the prod files that do not have a test file.
*
* @param matchedFiles list of all the files missing a test file
* @param testFileExemptiontextProto the location of the test file exemption textproto file
*/
private fun logFailures(matchedFiles: List<File>, testFileExemptiontextProto: String) {
if (matchedFiles.isNotEmpty()) {
matchedFiles.sorted().forEach { file ->
println("File $file does not have a corresponding test file.")
}
println("If this is correct, please update $testFileExemptiontextProto.textproto")
println(
"Note that, in general, all new files should have tests. If you choose to add an" +
" exemption, please specifically call this out in your PR description."
)
println()
}
}

/**
* Loads the test file exemptions list to proto.
*
* @param testFileExemptiontextProto the location of the test file exemption textproto file
* @return proto class from the parsed textproto file
*/
private fun loadTestFileExemptionsProto(testFileExemptiontextProto: String): TestFileExemptions {
val protoBinaryFile = File("$testFileExemptiontextProto.pb")
val builder = TestFileExemptions.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: TestFileExemptions =
FileInputStream(protoBinaryFile).use {
builder.mergeFrom(it)
}.build() as TestFileExemptions
return protoObj
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
"""
Tests corresponding to test file-related checks, such as ensuring that all the production
(all Kotlin files) files have a corresponding test file present.
"""

load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_jvm_test")

kt_jvm_test(
name = "TestFileCheckTest",
srcs = ["TestFileCheckTest.kt"],
deps = [
"//scripts:test_file_check_assets",
"//scripts/src/java/org/oppia/android/scripts/testfile:test_file_check_lib",
"//testing:assertion_helpers",
"//third_party:com_google_truth_truth",
"//third_party:org_jetbrains_kotlin_kotlin-test-junit",
],
)
Loading

0 comments on commit 9abd017

Please sign in to comment.