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

Improve multi-application experience, allow including suites by name (opposite of --skip) #6435

Merged
merged 4 commits into from
Jun 10, 2022

Conversation

calvinalkan
Copy link
Contributor

@calvinalkan calvinalkan commented Apr 18, 2022

Fix: #6434

This PR is a minimal solution to fix/support the issue described in #6434.

This allows running suites by name for included applications:

vendor/bin/codecept run -include-suite unit
vendor/bin/codecept run -i unit -i functional

Test cases are included.

@calvinalkan
Copy link
Contributor Author

I don't know why some remote coverage tests would fail for 7.2 only, nothing related has been changed there.

@calvinalkan
Copy link
Contributor Author

Master was already broken on tag 4.3.31 which I used as a base commit.

See here: https://github.com/snicco/Codeception/actions/runs/2185305288

@DavertMik
Copy link
Member

@calvinalkan thanks for the PR
I got the problem but I don't like the proposed solution and extra options added to CLI.

How about adding more verbose suite syntax instead, for instance

  • codecept run unit <- executes unit tests for current app
  • codecept run backed:unit <- executes unit tests for backend app
  • codecept run *:unit <- executes unit tests for app included apps (your case, as I understood)

This is just an idea but it is a more general solution and probably it can be implemented in a more beautiful way.

@calvinalkan
Copy link
Contributor Author

calvinalkan commented Apr 20, 2022

@DavertMik Thanks for the comment.

I agree the options are not ideal but this was the least change required solution.

codecept run backed:unit - What exactly should backend be here?

The relative path (from cwd) to a codeception.yml file I assume.

I'm using this in the context of a very big monorepo, so it would look like this for me:

codecept run src/packages/package-a:unit
codecept run src/packages/package-b:unit
codecept run src/packages/package-a:functional
codecept run src/packages/package-b:functional

Now running all tests for all packages by suite.

codecept run *:unit
codecept run *:functional

I like this solution a lot more than what I proposed.

Maybe we could even do:

codecept run src/packages/package-a unit (notice the space)

But this probably clashes a lot with how it currently works.

These changes should probably be done in the main method of the Run class right?

@calvinalkan
Copy link
Contributor Author

The only question is how should we run:

codecept run -i unit -i functional

Options:

  1. codecept run *:unit:functional
  2. ...?

@DavertMik
Copy link
Member

@calvinalkan If you want to run 2 suites it would be

codecept run *:unit,*:functional

as we have similar comma syntax for suites themselves:

codecept run unit,functioal

Sorry for delay in reply

@calvinalkan
Copy link
Contributor Author

@DavertMik Based on your feedback I implemented the following:

Runs unit and functional suites for all included apps
codecept run *::unit,*::functional

Runs unit and functional test for backend app
codecept run backend::unit,backed::functional

Same as above, but with nested package directory
codecept run /path/to/nested/package::unit

I chose using :: instead of : because : is already used for filtering. E.g (php vendor/bin/codecept run tests/acceptance/backend:^login)

@DavertMik
Copy link
Member

Good idea, thank you!
That will be awesome feature.

@calvinalkan
Copy link
Contributor Author

calvinalkan commented May 3, 2022

@DavertMik Any idea what's going on on PHP <= 7.1?

https://github.com/Codeception/Codeception/actions/runs/2263620973

@DavertMik
Copy link
Member

Please make pull request to 5.0 branch which drops php 7 support

@calvinalkan
Copy link
Contributor Author

I think I found the issue. On PHP7.1 it uses a prehistoric version of symfony/console which does not yet have the exit code constants.

This should work now.

I'd love to get this in 4.x as we are bound to it due to wp-browser not being there yet lucatume/wp-browser#571

@calvinalkan
Copy link
Contributor Author

@DavertMik All working now

src/Codeception/Command/Run.php Outdated Show resolved Hide resolved
@Naktibalda
Copy link
Member

@DavertMik This change is a feature. Should we bump version number to 4.2.0?

src/Codeception/Command/Run.php Outdated Show resolved Hide resolved
@calvinalkan
Copy link
Contributor Author

@DavertMik This change is a feature. Should we bump version number to 4.2.0?

That would be great.

@calvinalkan
Copy link
Contributor Author

Don't merge this yet. I found an issue that has no tests currently.

Or maybe it's not an issue but what about this scenario (might be an edge case):

What it the root codeception.yml file has some tests and includes other apps:

include:
  - src/Snicco/Component/*
  - src/Snicco/Bundle/*
paths:
  output: tests/_output
  tests: tests
  data: tests/_data
  support: tests/_support
settings:
  colors: true
bootstrap: bootstrap.php
reporters:
  report: "PhpStorm_Codeception_ReportPrinter"

This would be for example the case with a monorepo where you have some root level tests for utilities related to the monorepo and each included app has its own separate test suites.

On my fork currently when you run (with the config above)

codecept run unit the following happens:

  • the unit suite from the root repo is run, all other suites from included apps are run. This is not desired of course and I think we can prevent this by just adding a check for a suite here.

Ill fix that now,

The last question is:

When I run codecept run *::unit, should that include the unit suite of the root application (if present). I would say yes.

@calvinalkan
Copy link
Contributor Author

@DavertMik @Naktibalda

I added some new tests that now verify previously uncovered behavior:

Tests are pretty self-explanatory but in resume:

  • codecept run => Everything is run
  • codecept run unit => Runs unit suite from current app
  • codecept run *::unit => Runs unit suites from all included apps and NOT the root suite
  • codecept run unit,*::unit => Runs included unit suites AND root unit suite
  • codecept run functional,*::unit => Runs included unit suites and root functional suite

I think this is the most flexible approach.

@Naktibalda Naktibalda changed the base branch from 4.1 to 4.2 May 14, 2022 08:36
@Naktibalda
Copy link
Member

Please rebase your test on 4.2 branch and fix merge conflict in IncludedCest.

Copy link
Member

@Naktibalda Naktibalda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code style issues

src/Codeception/Command/Run.php Outdated Show resolved Hide resolved
src/Codeception/Command/Run.php Outdated Show resolved Hide resolved
src/Codeception/Command/Run.php Outdated Show resolved Hide resolved
src/Codeception/Command/Run.php Outdated Show resolved Hide resolved
@calvinalkan
Copy link
Contributor Author

@Naktibalda Rebased onto 4.2

Copy link
Member

@DavertMik DavertMik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!
Looks good to me!

@calvinalkan
Copy link
Contributor Author

@DavertMik Now we only have to fix running failed tests in multi-app and then it's pretty awesome for monorepos.

@@ -3,8 +3,6 @@

use Codeception\Codecept;
use Codeception\Configuration;
use Codeception\Lib\GroupManager;
use Codeception\Util\PathResolver;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputArgument;
use Symfony\Component\Console\Input\InputInterface;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document new syntax in Usage section of class level docblock in this file. (Line 24)

@Naktibalda
Copy link
Member

@DavertMik Now we only have to fix running failed tests in multi-app and then it's pretty awesome for monorepos.

Does this change break some tests?

@Naktibalda Naktibalda merged commit 1316cdc into Codeception:4.2 Jun 10, 2022
@calvinalkan
Copy link
Contributor Author

@DavertMik Now we only have to fix running failed tests in multi-app and then it's pretty awesome for monorepos.

Does this change break some tests?

@Naktibalda I don't know, I have no solution yet and haven't really looked. I just know it doesn't work correctly. Probably related to the failed groups being written to a wrong location. I ll open a separate Issue/PR once I know more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants