Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Translation] ReworkPhpAstExtractor tests organization for future improvements#60854
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
[Translation] ReworkPhpAstExtractor tests organization for future improvements#60854
Uh oh!
There was an error while loading.Please reload this page.
Conversation
19b3d28 to2ed841fCompare| use Symfony\Component\Translation\Extractor\PhpAstExtractor; | ||
| use Symfony\Component\Translation\MessageCatalogue; | ||
| abstract class AbstractVisitorTest extends TestCase |
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.
Should be namedAbstractVisitorTestCase as PHPUnit 10 has deprecated the fact of having abstract classes matching the class name suffix of tests (which isTest by default, and we use the default), and PHPUnit 11 fails for such case.
| abstract class AbstractVisitorTest extends TestCase | ||
| { | ||
| abstract public function getVisitor(): NodeVisitor; | ||
| abstract public function getResource(): iterable|string; |
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.
this should have pphdoc for 2 reasons:
- document the return type fully
- explain what this method is about
| use Symfony\Component\Translation\Extractor\Visitor\ConstraintVisitor; | ||
| use Symfony\Component\Translation\MessageCatalogue; | ||
| class ConstraintVisitorTest extends AbstractVisitorTest |
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.
having to make a new test class for each test looks weird to me.
we might want to have multiple tests running for ConstraintVisitor for instance, which would then require finding names for those test classes (instead of naming test methods like usual) or to merge everything in a single test (which removes any explanation of theintent of the test, and also prevents testing some legacy feature and non-legacy ones separately)
Designing theAbstractVisitorTest testcase in a way supporting a single test does not really matches the common PHPUnit architecture.
You should rather make it define a utility method taking a NodeVisitor and a resource as argument and returning a MessageCatalogue, and let each test class write as many tests it wants using this utility.
| @@ -0,0 +1,72 @@ | |||
| This template is used for translation message extraction tests | |||
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.
why using a PHP template to cover the case of extracting from form types ? This does not make sense to me.
| @@ -0,0 +1,72 @@ | |||
| This template is used for translation message extraction tests | |||
| <?php | |||
| // @see https://github.com/php-translation/extractor/blob/master/tests/Resources/Php/Symfony/ExplicitLabelType.php | |||
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.
why a@see targetting an external file (with a URL that won't even stay valid as it referencemaster which is a mutable reference)
| @@ -0,0 +1,47 @@ | |||
| This template is used for translation message extraction tests | |||
| <?php new TranslatableMessage('translatable single-quoted key'); ?> | |||
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.
isn't this missing a use statement ?
2ed841f to23ead95Comparesrc/Symfony/Component/Translation/Extractor/Visitor/AbstractVisitor.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
…sts-rework-review-0001[Translation] Rework PhpAstExtractor tests organization for future improvements - REVIEW0001
PhpAstExtractor tests organization for future improvements
This PR is the first in a series that tend to improve PhpAstExtractor (covering more extractions cases, ease test writting for new cases, etc).
I've worked with@Jean-Beru,@Kazadri and@jprivet-dev in the past few weeks on various new cases to increase the number of "locations" from where the extractor can extract translation keys.
As we were working on my own Symfony fork, I'm opening the PR.
But to be honest my colleague@Jean-Beru did the large majority of the work here. Thanks to him 👏!
To avoid a big PR containing multiple new small features, we need to open and hopefully merge this one first to ease the tests writting for next new Visitors.
NB: I'm not sure to qualify or not this PR as New feature?