[mustache_template] Fix auto-generated specification tests, run with dart test#11056
[mustache_template] Fix auto-generated specification tests, run with dart test#11056jslater89 wants to merge 9 commits intoflutter:mainfrom
dart test#11056Conversation
Calling the other 'main' functions would run all the tests from parser_test and feature_test from mustache_test.dart as well as their standalone files.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring of the mustache_template test suite. By reorganizing the tests to be runnable with dart test, introducing a script to manage the mustache specifications, and cleaning up the test structure, you've greatly improved the maintainability and usability of the tests. The changes are well-documented in the new README.md. I have a couple of suggestions to further improve the new download script and enhance consistency within the test code.
| git clone https://github.com/mustache/spec.git tmp_spec | ||
|
|
||
| HEAD_HASH=$(git -C tmp_spec rev-parse HEAD) | ||
| UTC_NOW=$(date -u +%Y-%m-%dT%H:%M:%SZ) | ||
|
|
||
| if [ -d "specs" ]; then | ||
| rm -rf specs | ||
| fi | ||
| mkdir -p specs | ||
|
|
||
| mv tmp_spec/specs/* specs/ | ||
| rm -rf tmp_spec | ||
|
|
||
| echo "Specs commit: $HEAD_HASH" > specs/meta.txt | ||
| echo "Specs download date: $UTC_NOW" >> specs/meta.txt |
There was a problem hiding this comment.
This script is a great addition for managing the spec files. To make it more robust and slightly simpler, I suggest adding a shebang and set -e at the top. The shebang ensures the script is run with the correct interpreter, and set -e will cause the script to exit immediately if any command fails, preventing potential issues.
I've also simplified the directory removal logic, as rm -rf doesn't error if the directory doesn't exist, making the if check redundant.
| git clone https://github.com/mustache/spec.git tmp_spec | |
| HEAD_HASH=$(git -C tmp_spec rev-parse HEAD) | |
| UTC_NOW=$(date -u +%Y-%m-%dT%H:%M:%SZ) | |
| if [ -d "specs" ]; then | |
| rm -rf specs | |
| fi | |
| mkdir -p specs | |
| mv tmp_spec/specs/* specs/ | |
| rm -rf tmp_spec | |
| echo "Specs commit: $HEAD_HASH" > specs/meta.txt | |
| echo "Specs download date: $UTC_NOW" >> specs/meta.txt | |
| #!/bin/bash | |
| set -e | |
| git clone https://github.com/mustache/spec.git tmp_spec | |
| HEAD_HASH=$(git -C tmp_spec rev-parse HEAD) | |
| UTC_NOW=$(date -u +%Y-%m-%dT%H:%M:%SZ) | |
| rm -rf specs | |
| mkdir -p specs | |
| mv tmp_spec/specs/* specs/ | |
| rm -rf tmp_spec | |
| echo "Specs commit: $HEAD_HASH" > specs/meta.txt | |
| echo "Specs download date: $UTC_NOW" >> specs/meta.txt |
| expect(ex is TemplateException, isTrue); | ||
| expect( | ||
| (ex! as TemplateException).message, | ||
| startsWith(BAD_VALUE_INV_SECTION), | ||
| ); |
There was a problem hiding this comment.
For consistency with other tests in this file (e.g., 'Mismatched tag' on line 396), you can use the existing expectFail helper function here. This will make the test more concise and readable by abstracting the exception checking logic.
| expect(ex is TemplateException, isTrue); | |
| expect( | |
| (ex! as TemplateException).message, | |
| startsWith(BAD_VALUE_INV_SECTION), | |
| ); | |
| expectFail(ex, null, null, BAD_VALUE_INV_SECTION); |
While waiting for review, FYI unit tests are failing in CI on several platforms, which will need to be addressed. |
|
Whoops, so they are. The web tests are failing because the specification test generator relies on dart:io; that would have been broken before too. I'll still fix that if needed. The Windows failure is probably me being a dope about path separators. I'll correct the formatting issues too when I do the others, some night this week. |
Except that the stated purpose of the PR is:
If something wasn't run in CI before, then it wasn't broken in CI before.
I'm not sure what you mean by "if needed". Isn't the goal to land the PR? We can't land a PR that breaks CI. |
The handwritten 'null inverted section' test expected no output from
{{^section}} content {{/section}} when {'section': null} in the context,
but specs/inverted/Null is falsey indicates that the content should
render in that case.
3 participants
This PR reorganizes the tests in mustache_template such that CI can run them, per the second option proposed in flutter/flutter#174721. The changes include a script to pull mustache specifications from the mustache repository and note the time/date and commit hash of that action, along with documentation of the above and the new test structure laid out below.
The former
all.dart(which called threemainfunctions in three test files) is nowmustache_test.dart, and the formermustache_test.dart(which contained handwritten tests) is nowfeature_test.dart. Themustache_specs.dartfile is now solely responsible for generating the tests inmustache_test.dartfrom the mustache specs, and no longer has amainof its own.As this implementation doesn't support two of the optional mustache specs (inheritance and dynamic names, which were added to the mustache repo in the early 2020s, while the original mustache_template package was written in the mid-2010s), this PR also adds an UNSUPPORTED_SPECS constant in
mustache_test.dartthat skips generation of tests from those spec files.(Separately, I'm working on an implementation of the inheritance spec, so it seemed better to pull all the specification files and selectively disable the unsupported ones.)
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).Exemptions
If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3