-
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 #3291: Add check for XML syntax validation #3341
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
Same comment from the previous PR review, for script kt
file and test file.
.github/workflows/static_checks.yml
Outdated
|
||
- name: XML Syntax Validation Check | ||
run: | | ||
bazel run //scripts:xml_syntax_check -- $(pwd) app data utility domain |
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.
data domain doesn't have any res file, utility does file dimen and string XML file if we are checking that else no needed, we can keep the check only for app module.
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.
Yes! they don't have any res files, but my only concern was that these layers do have the AndroidManifest.xml
file present, so I thought of including them too in the XML syntax check.
WDYT?
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.
Looking for @BenHenning thoughts on this
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.
Hmm. I just left a comment in #3340 about passing in the root directory and just recursing among that. If we validate all XML files then this slight oddity is less noticeable (since we do ultimately want all XML files to be valid). Would that mitigate the concern here?
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.
Done.
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! Same as #3340. This is effectively 'LGTM' with one more comment to resolve. Please resolve that & assign back once all CI tests are passing in this PR.
scripts/src/javatests/org/oppia/android/scripts/xml/XmlSyntaxErrorHandlerTest.kt
Show resolved
Hide resolved
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, but waiting to approve until #3340 is merged. Also, per our discussion I'm cancelling the workflows for this & the next PR since that will relieve pressure on the CI system and once you merge #3340 into develop and update these PRs they should have much fewer tests to run..
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! CI is green so going ahead and merging this.
Explanation
Fixes #3291: Add check for XML syntac validation
The script traverses different layers of the codebase to check for the syntax correctness of XML files
To run the script, use:
bazel run //scripts:xml_syntax_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/xml:XmlSyntaxCheckTest
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: #3340
Screenshot of new tests passing locally:
Checklist