Skip to content
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

Add winter:test command #202

Merged
merged 22 commits into from
Oct 21, 2021
Merged

Add winter:test command #202

merged 22 commits into from
Oct 21, 2021

Conversation

RomainMazB
Copy link
Contributor

First of the console command discussed here #201

Accepts any review from anyone out there.

I did not found how it would be possible to display the phpunit running test. As of today it runs the tests "silently" and then output the results.

plugin-test-command.mp4

@bennothommo
Copy link
Member

bennothommo commented Jun 9, 2021

@RomainMazB it's definitely possible to get the output happening in real-time - see the Dusk command class here, which does the same thing for Dusk tests: https://github.com/wintercms/wn-dusk-plugin/blob/main/console/Dusk.php. I would prefer that the output be in real-time if possible.

The key is to use the Process component from Symfony.

@bennothommo bennothommo added Status: Revision Needed enhancement PRs that implement a new feature or substantial change labels Jun 9, 2021
@RomainMazB
Copy link
Contributor Author

@bennothommo sure, I'd definitely prefer too. Thanks for pointed out how to do this, I'll definitely implement this

@LukeTowers
Copy link
Member

Might be nice to have it as winter:test ?target where target could be blank for the application's test suite or a specific plugin, and also forward any arguments / options / filters to the phpunit command (example: I use the --filter option a lot to run a specific single test)

@RomainMazB RomainMazB marked this pull request as draft June 30, 2021 07:58
@RomainMazB RomainMazB marked this pull request as ready for review June 30, 2021 20:54
@RomainMazB
Copy link
Contributor Author

Ok, improvements made.
@LukeTowers to proxy all the phpunit filter we are forced to ignore validation (or create a 50 lines signature to describe all the available options). The same is done into the Laravel's core test command: https://github.com/nunomaduro/collision/blob/stable/src/Adapters/Laravel/Commands/TestCommand.php#L48-L53

A quick view of the final results.
https://user-images.githubusercontent.com/53976837/124030370-68e5b980-d9f6-11eb-893f-c6d57cc45b58.mp4

@RomainMazB RomainMazB changed the title Add plugin:test command Add winter:test command Jun 30, 2021
@LukeTowers LukeTowers requested a review from bennothommo June 30, 2021 23:04
Co-authored-by: Luke Towers <github@luketowers.ca>
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
@bennothommo bennothommo added this to the v1.1.5 milestone Jul 1, 2021
@LukeTowers
Copy link
Member

@RomainMazB does the core tests differentiate between the CMS tests and the library tests? @bennothommo should we have a way to trigger them separately or together?

@bennothommo
Copy link
Member

@LukeTowers we could define all the tests as separate suites in the phpunit.xml file and it would allow us to run all tests together, but there's no way to separate the test results - not sure if that's necessary a problem for people.

We used to combine the library and core tests, but I separated them during the L6 update because tests were failing when core PRs were relying on unmerged library PRs.

RomainMazB and others added 6 commits July 1, 2021 20:44
Co-authored-by: Ben Thomson <ben@abweb.com.au>
Co-authored-by: Ben Thomson <ben@abweb.com.au>
Co-authored-by: Ben Thomson <ben@abweb.com.au>
Co-authored-by: Ben Thomson <ben@abweb.com.au>
Co-authored-by: Ben Thomson <ben@abweb.com.au>
Co-authored-by: Ben Thomson <ben@abweb.com.au>
@RomainMazB RomainMazB marked this pull request as draft July 1, 2021 19:21
@RomainMazB
Copy link
Contributor Author

In the latest commit, we can now pass a custom configuration file using --configuration option.

If none is defined, it looks for phpunit.xml file and fallback to phpunit.xml.dist.

Need precision about my comment on @bennothommo review about exists() and the final thoughts on the library and core tests to continue.

@github-actions
Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

@github-actions github-actions bot added the stale Issues/PRs that have had no activity and may be archived label Sep 20, 2021
@LukeTowers LukeTowers removed the stale Issues/PRs that have had no activity and may be archived label Sep 20, 2021
@jaxwilko
Copy link
Member

I've made a few changes to improve the arguments passthrough to phpunit, there is an issue where if phpunit arguments are passed before the command arguments then the command args aren't parsed.

Seems to be an issue from Symfony\Component\Console\Input\ArgvInput. I shall look into it and see what I can find :)

@RomainMazB
Copy link
Contributor Author

@LukeTowers @bennothommo.
I've been toying with the ArgvInput symfony component to find the cause of the issue mentioned by @jaxwilko.

The issue is not directly the fact that we pass a PHPUnit argument before --plugin or --core, but the fact the winter:test command doesn't define those options, which raise a RuntimeException, the exception is silently hidden by the ignoreValidationErrors rule we are using and present in the Symfony Command class here: https://github.com/symfony/symfony/blob/e34cd7dd2c6d0b30d24cad443b8f964daa841d71/src/Symfony/Component/Console/Command/Command.php#L248-L252

This makes the options to not be correctly set and runs the command without any options at all. I'm afraid that we won't be able to allow phpunit options before the winter:test command's ones.

I've added a section in the documentation emphasizing the fact that phpunit options should be passed after the command's options. which works well:
image

@RomainMazB RomainMazB marked this pull request as ready for review October 19, 2021 09:05
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
modules/system/console/WinterTest.php Outdated Show resolved Hide resolved
No need to include docblock comments for information provided by type hints unless additional comments are made that justify a docblock comment
@LukeTowers LukeTowers merged commit 345f02e into wintercms:develop Oct 21, 2021
LukeTowers added a commit that referenced this pull request Nov 13, 2021
* develop: (25 commits)
  Support embedded data URIs in the list image column type
  Make some adjustments to the readme content
  Update banner in readme
  Add new GitHub banner
  Documentation with icons (#347)
  Limit options shown in group filter, apply scope when retrieving filtered options
  Add Exception on wrong relation type in relation formwidget (#334)
  Redesigned color picker widget (#324)
  Add winter:test command (#202)
  Use the correct backend timezone config key (#337)
  Get changelog only of the current branch
  Fix Markdown editor sizing issue on Chrome.
  Check overrides for parent locale when compiling language files (#242)
  Fixing commas in English translation files (#305)
  Added Latvian translations for Allowed IP messages (#304)
  Add missing filter translations (#303)
  Clean up newlines
  Update Russian language (#302)
  Fix issue present in overriding RelationController partials using the default code
  Maintenance Allowed IP list (#147)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PRs that implement a new feature or substantial change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants