-
Notifications
You must be signed in to change notification settings - Fork 521
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 #3292: Add check for test files presence for prod files #3343
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @Sparsh1212
The same comment applies here from your previous PR regarding the script .kt
file and its test file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Sparsh1212. Generally LGTM, but two questions:
- Can you please bring this branch up-to-date with develop now that Fix #3291: Add check for XML syntax validation #3341 is merged & update the title to no longer indicate it's blocked
- Can you clear the textproto file & show what the CI failure looks like for an empty exemption list? I'm interested in verifying the failure cases in a real CI scenario
- After (2), please restore the textproto file so that CI is green (the result should be 1 commit with the specific failures requested from (2) and the latest commit passing)
scripts/src/java/org/oppia/android/scripts/proto/script_exemptions.proto
Outdated
Show resolved
Hide resolved
@BenHenning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Sparsh1212. LGTM! Just waiting for green CI to merge.
Unassigning @BenHenning since they have already approved the PR. |
Assigning @seanlip for code owner reviews. Thanks! |
@Sparsh1212 looks like the new check is failing in CI--please fix. |
I'm a bit confused ... why is Oppiabot assigning me to this PR? I don't code-own any files in it. |
@seanlip looks like a potential bug. You were auto-added as a reviewer because a base PR changed the codeowners file. I guess Oppiabot assumes that reviewers always get auto-assigned even if they aren't a codeowner or at-mentioned? |
Thanks @Sparsh1212. Everything is looking good. Enabling auto-merge. |
Oh, weird. Hm, I'm not too bothered about it, but if it recurs it might be worth fixing on Oppiabot's end. /cc @jameesjohn for info. |
Yes. oppiabot assumes that reviewers who are yet to review a PR are to be assigned. |
Explanation
Fixes #3292: Add check for test files presence for prod files
The script traverses different layers of the codebase to check for the presence of test files for prod files.
Note: It works on the logic of searching Filename+'Test'.kt in the codebase, to ensure the test file presence.
The files listed in the
test_file_exemptions.textproto
will be skipped for this check, and currently, all the prod files which were failing this check, have been added to this list so that the workflow doesn't fail.We can remove any file from the exemption list, where we think that we can't go without a test file for that prod file, or if a test file is now added for that particular file.
To run the script, use:
bazel run //scripts:test_file_check -- $(pwd)
For testing the script, automated tests have been added.
To execute the tests, use:
bazel test //scripts/src/javatests/org/oppia/android/scripts/testfile:TestFileCheckTest
Note: We are generating the test assets dynamically at the time of executing them. The test assets are automatically deleted, when the test finishes.
Blocked on: #3341 #3340
Screenshot of all new added tests passing successfully on local:
Checklist