Fix #3290: Add support for generic regex pattern matching #3328
Conversation
Sparsh1212
requested review from
anandwana001,
fsharpasharp and
vinitamurthi
as code owners
There was a problem hiding this comment.
DummFile2 should be something valid that does not contains any support library.
Also, rather than using a dummy file name, we can use something more descriptive name.
Add some more description in the PR description on what are the patterns we are testing here.
Thanks @Sparsh1212
|
|
||
| // Error to show if any files match the regex. | ||
| string failure_message = 2; | ||
| } |
| @@ -0,0 +1 @@ | |||
| import android.support.v7.app | |||
There was a problem hiding this comment.
Add something valid file
There was a problem hiding this comment.
Done regarding eof.
Regarding what content we should add here, I am not sure, as this is just meant to be for the path pattern check.
Adding @BenHenning for additional thoughts.
| filename_regex: ".+?.kt" | ||
| prohibited_content_regex: "^import .+?support.+?$" | ||
| failure_message: "AndroidX should be used instead of the support library" | ||
| } |
There was a problem hiding this comment.
Also, we can name something which resemble more on prohibiting the use of support library.
scripts/src/java/org/oppia/android/scripts/RegexPatternValidationCheck.kt
Show resolved
Hide resolved
| class RegexPatternValidationCheck { | ||
| companion object { | ||
| @JvmStatic | ||
| fun main(vararg args: String) { |
There was a problem hiding this comment.
name should be more descriptive
There was a problem hiding this comment.
also, we don't need to use JvmStatic
There was a problem hiding this comment.
Actually, here it looks like we need it in order to run the kt_jvm_binary.
I tried removing it, but when running the scripts, it gave me an error, that it wasn't able to recognize the static main method.
There was a problem hiding this comment.
Did you try to change the method name?
There was a problem hiding this comment.
The method name must be main only. as it is necessary for the kt_jvm_binary to run the script. I did this from the reference PR https://github.com/oppia/oppia-android-assets/pull/7/files
In this it is done in the same way.
| fun prohibited_Content_Check_Should_Fail() { | ||
| assertEquals( | ||
| true, | ||
| RegexPatternValidationCheck.checkProhibitedContent( |
| @Test | ||
| fun prohibited_Content_Check_Should_Fail() { | ||
| assertEquals( | ||
| true, |
| repoPath = repoPath, | ||
| searchFiles = searchFiles, | ||
| fileNameRegexString = "DummyFile1.kt", | ||
| prohibitedContentRegexString = "^import .+?support.+?$", |
There was a problem hiding this comment.
can we get this string from some single utility.
There was a problem hiding this comment.
Updated, to fetch it from the textproto itself.
| searchFiles = searchFiles, | ||
| fileNameRegexString = "DummyFile1.kt", | ||
| prohibitedContentRegexString = "^import .+?support.+?$", | ||
| errorToShow = "AndroidX should be used instead of the support library" |
| } | ||
|
|
||
| @Test | ||
| fun no_Prohibited_Content_Check_Should_Pass() { |
|
Hi @Sparsh1212, it looks like some changes were requested on this pull request by @anandwana001. PTAL. Thanks! |
|
Removing my assignee as I already reviewed it first time. |
|
@anandwana001 Thanks for the review, |
|
@fsharpasharp Can you please take a pass for the Bazel files. |
Assigning you back as there are few comments which I suggested earlier that are not addressed. |
|
Unassigning @fsharpasharp since they have already approved the PR. |
Apologies! I missed some of the nits. |
| - uses: actions/checkout@v2 | ||
|
|
||
| - name: Set up Bazel | ||
| uses: abhinavsingh/setup-bazel@v3 |
There was a problem hiding this comment.
Do we want to be using actions that are created by individuals? @BenHenning
There was a problem hiding this comment.
We are currently using this action in the unit_tests.yml workflow:
scripts/BUILD.bazel
Outdated
| kt_jvm_library( | ||
| name = "pattern_validation_lib", | ||
| testonly = True, | ||
| srcs = glob(["src/java/org/oppia/android/scripts/*.kt"]), |
There was a problem hiding this comment.
Can we avoid using glob? In general its better to explicitly list down the files rather than giving a generic rule like this
scripts/BUILD.bazel
Outdated
| kt_jvm_test( | ||
| name = "test_pattern_validation_check", | ||
| testonly = True, | ||
| srcs = glob(["src/test/org/oppia/android/scripts/RegexPatternValidationCheckTest.kt"]), |
There was a problem hiding this comment.
Same comment here and below
| proto_library( | ||
| name = "filename_pattern_validation_structure_proto", | ||
| srcs = ["filename_pattern_validation_checks.proto"], | ||
| visibility = ["//visibility:public"], |
There was a problem hiding this comment.
Do we need visibility to be public? Again, we like to avoid using public as much as possible. Is it possible to provide a set of subpackages for visibility here and below?
| option java_multiple_files = true; | ||
|
|
||
| message FileContentChecks { | ||
| repeated FileContentCheck file_content_checks = 1; |
There was a problem hiding this comment.
Add a comment explaining the field
| option java_package = "org.oppia.android.app.model"; | ||
| option java_multiple_files = true; | ||
|
|
||
| message FileContentChecks { |
There was a problem hiding this comment.
Add a comment explaining the message
| repeated FileContentCheck file_content_checks = 1; | ||
| } | ||
|
|
||
| message FileContentCheck { |
There was a problem hiding this comment.
Add a comment explaining the message
| option java_package = "org.oppia.android.app.model"; | ||
| option java_multiple_files = true; | ||
|
|
||
| message FilenameChecks { |
There was a problem hiding this comment.
Add comments here and below
|
@vinitamurthi Addressed all your suggestions. Please take a look. |
|
@anandwana001 @vinitamurthi @fsharpasharp @BenHenning |
Explanation
Fixes #3290: Add support for generic regex pattern matching
The script traverses different layers of the codebase to check for any prohibited file content and filenaming pattern.
To run the script:
bazel run //scripts:pattern_validation_check -- $(pwd)Tests are added to check for the prohibited file content and filenaming patterns in the
scripts/src/test/testfilesdirectory.To run the tests:
bazel test //scripts:test_pattern_validation_checkThe tests are designed in a way such that, we analyze the script checks over some dummy testfiles. The testfiles directory has dummy test files present for all the checks under their respective directory. They are further segregated into two directories:
failpassfaildirectory contains the files, which will trigger failure for the script checks.passdirectory contains the files, which will make the checks pass.Checklist