Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
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 support for Codeception Test Framework #800

Merged
merged 25 commits into from Nov 27, 2019
Merged

Conversation

@maks-rafalko
Copy link
Member

maks-rafalko commented Oct 2, 2019

This PR adds support for Codeception Testing Framework as this is the most requested feature in Infection issues.

Before reviewing, I would like to describe a couple of things so please make sure read them.

Why another PR?

There were 2 PRs trying to add support for Codeception:

  1. #143
  2. #457

Unfortunately, none of them could work (not even close). I will explain below why.

However, @tobiasstadler & @Fenikkusu did a great job moving the whole process forward and patching Codeception itself. Thank you guys!

Explanation

Previous PRs:

  • Did't work at all because of blokers on Codeception side
  • had no code for working wiht PHAR
  • had many architecture issues that were not addressed (my and others comments)
  • had the architecture of the code which was not in line with the other test frameworks implemented in Infection
  • had nothing about using JUnit and other things to improve performance

So, instead of fixing all those issues I decided to start from scratch, borrowing some ideas from that 2 PRs.

What was done

During the integration, there was 2 types of issues: blockers and things that are very annoying in Codeception that led me write strange code.

Blockers:

  • [RESOLVED in 3.1.0] It was not possible to have one bootstrap file in Codeception. Fortunately, we fixed the issue in Codeception. Without this change it's impossible (or extremely hard to) using Codeception with Infection. (we are using bootstrap file to add Interceptor)
  • [RESOLVED in 3.1.1] Ability to use absolute and relative paths in groups Codeception/Codeception#5674. This was needed to run particular tests that cover mutated line of the code. Without this, running all the tests for each Mutant is practically useless.

Annoying issues:

These issues are workarounded in the code, but fixing them in the Codeception will make the Infection code a lot easier & understandable. These are not blockers though.

Implementation details

This PR adds integration with Codeception ^3.1.1. Working with the previous versions potentially possible, but requires extremely huge amount of time, dirty workarounds to avoid Codeception bugs. I'm not sure we need it, at least for now.

Codeception adapter supports:

  • memory detector (as PHPUnit does)
  • uses JUnit report. It means that Infection
    • runs only subset of tests that cover mutated line.
    • sorts those tests and runs fastest first

BC Break

The name of the JUnit report file has been changed from phpunit.junit.xml to junit.xml. This is done to have common approach for PHPUnit / Codeception and any other future Test Framework.

Now, providing code coverage for PHPUnit and Codeception requires exaclty the same files.

Future plans

I'm really going to extract Test Framework Adapters to the separate packages. And Codeception is a good evidence that this must be done. Why? One of the many reasons: because we can't say in Infection's composer.json that we require: codeception/codeception:^3.1.1 because not all people use codeception as a test framework. Instead, we should introduce infection/core and infection/codeception, infection/phpunit (names are just to understand the goal). More on it in a separate issue.

Last words: I'm not going to support every project's codeception test suite because there are tons of settings and cases in Codeception where Infection can fail. IMO we should stabilize this basic implementation, merge it, and keep improving it step by step to not have this PR open for ages. Unfortunately, this takes too much time (debugging Codeception itself and patching it).


This change is Reviewable

@Fenikkusu

This comment has been minimized.

Copy link
Contributor

Fenikkusu commented Oct 2, 2019

Following. Unfortunately was never able to get back to this. Spending 60+ hours in your day job tends to have that effect.

@maks-rafalko maks-rafalko added the feature label Oct 2, 2019
@maks-rafalko maks-rafalko added this to the 0.15.0 milestone Oct 2, 2019
Copy link
Member

BackEndTea left a comment

Great work @maks-rafalko, i got some minor nit picks right now, but i'll try and really check it out sometime soon

final class CodeceptionAdapterTest extends TestCase
{
/**
* @var CodeceptionAdapter|MockObject

This comment has been minimized.

Copy link
@BackEndTea

BackEndTea Oct 8, 2019

Member

CodeceptionAdapter&MockObject is more correct here.

This comment has been minimized.

Copy link
@maks-rafalko

maks-rafalko Oct 8, 2019

Author Member

I get

PHPDoc tag @var for property Infection\Tests\TestFramework\Codeception\Adapter\CodeceptionAdapterTest::$adapter contains unresolvable type.

we don't use https://github.com/phpstan/phpstan-phpunit because we use PHPStan PHAR at the moment

This comment has been minimized.

Copy link
@BackEndTea

BackEndTea Oct 8, 2019

Member

Have you tried reseting cache in between that? I've been having some issues with phpstan and cache of doc annotations lately

This comment has been minimized.

Copy link
@maks-rafalko

maks-rafalko Oct 9, 2019

Author Member

Where is the phpstan cache located? I don't see anything related to it in our command lines and in the --help output.

I tried to run phpstan from the different folder - same result

This comment has been minimized.

Copy link
@BackEndTea

BackEndTea Oct 10, 2019

Member

I dont' really know where it stores it by default, my best guess would be somewhere in /tmp

@dirx

This comment has been minimized.

Copy link

dirx commented Oct 17, 2019

Looks very promising! I tested the current state and run into relative path issue with DB module and the dump path(s) configuration - reading sql files is based on the codeception project dir.

@maks-rafalko

This comment has been minimized.

Copy link
Member Author

maks-rafalko commented Oct 17, 2019

Ok, could you please create a minimum reproducable repository I can look at? or is it a public repo?

@dirx

This comment has been minimized.

Copy link

dirx commented Oct 17, 2019

No public repo - maybe this helps to dig into this:

Codeception sets its project dir based on the config yml file:
https://github.com/Codeception/Codeception/blob/3.1/src/Codeception/Configuration.php#L158

In DB Module project dir is then used as base path to read sql files:
https://github.com/Codeception/Codeception/blob/3.1/src/Codeception/Module/Db.php#L497

Running codecept with initial / mutation yml configurations in infection tmp path then sets "wrong" relative project dir.

@maks-rafalko

This comment has been minimized.

Copy link
Member Author

maks-rafalko commented Oct 17, 2019

Not enough information unfortunately. Could you please at least show your folder structure and codeception.yaml as well as other configs (if any) from the nested tests dir?

@dirx

This comment has been minimized.

Copy link

dirx commented Oct 17, 2019

@maks-rafalko

This comment has been minimized.

Copy link
Member Author

maks-rafalko commented Oct 19, 2019

Thank you very much for creating a repo, that helped a lot. Really appreciate it.

Unfortunately, you've discovered a serious problem. Let me explain in details (would also be useful for those who don't know how Codeception works with config files).

So, we have the following nested configs in your repo:

├── codeception.yml
└── tests
    ├── _data
    ├── _output
    ├── _support
    ├── integration
    └── integration.suite.yml

It means that Codeception merges content of integration.suite.yaml into codeception.yaml. It sounds like not a complex stuff, but let's see how infection works.

Infection replaces only main codeception.yaml config when runs the tests and Mutants:

codecept run --config /app/./infection/codeception.initial.infection.yml [...]

But tests/integration.suite.yaml remains untouched. When Codeception do its job (running from Infection process), it still scans all folders and merges suites' configs into the main overridden /app/./infection/codeception.initial.infection.yml file.

That's why all paths from codeception.yaml are correctly amended with prefixes, but all from tests/* are not.

I think we can't do anything with it, without seriously patching Codeception itself.

But, I found something interesting. We can declare all suites in the main codeception.yaml file! See here: https://codeception.com/05-22-2017/codeception-2-3.html#algolia:p:nth-of-type(15)

This is what I did in your repository, and with the latest commit it works!

  • removed tests/integration.suite.yml file
  • added suite's configuration to codeception.yml
# codeception.yaml

[..]

suites:
    integration:
        actor: IntegrationTester
        modules:
            enabled:
                - \Stuff\Helper\Integration
                - Db:
                      dsn: 'sqlite:stuff.sq3'
                      user: 'test'
                      password: 'test'
                      dump:
                          - 'tests/_data/stuff.sql'
                      cleanup: false
                      populate: true
            step_decorators: ~

I would say that moving suites's configuration to the main file can be a good solution without any changes in the code of Codeception and Infection itself. Otherwise, we need to think about what we can do with nested configuration files that are merged by Codeception itself, which can be a very difficult task.

Please, pull the latest commit and let me know if it works for you! Thank you.

@dirx

This comment has been minimized.

Copy link

dirx commented Oct 20, 2019

@maks-rafalko thank you for taking a deeper look into this. Looks like this workaround might work, but probably not for other codeception config scenarios. For example "extends:" would not work without rewriting paths again.

Rewriting paths seems to bring a lot of trouble.

Why not staying with the original codeception config (and path) and overriding config values needed for the initial and all the mutation runs?

Initial command line might look like:

vendor/codeception/codeception/codecept run --no-colors --fail-fast \
--config ./codeception.yml \
--coverage-phpunit ./infection/codeception-coverage-xml \
--xml ./infection/junit.xml \
--override 'extends: ./infection/codeception.initial.infection.yml'

Mutation command line might look like:

vendor/codeception/codeception/codecept run --no-colors --fail-fast \
--config ./codeception.yml \
--coverage-phpunit ./infection/codeception-coverage-xml \
--xml ./infection/junit.xml \
--override 'extends: ./infection/codeceptionConfiguration.03a5c2544152d2cbf1e679057880c806.infection.yaml'

Unfortunately Codeception does currently not support extends in cli override - it just appends the overriding config value without resolving it beforehand (feature might be easy to add, though).

Until then config values have to be explicitly overridden:

Initial command line:

vendor/codeception/codeception/codecept run --no-colors --fail-fast \
--config ./codeception.yml \
--coverage-phpunit ./infection/codeception-coverage-xml \
--xml ./infection/junit.xml \
--override 'paths: output: ./infection' \
--override 'coverage: enabled: true' \
--override 'coverage: include: ["src/*.php"]'

Mutation command line:

vendor/codeception/codeception/codecept run --no-colors --fail-fast \
--config ./codeception.yml \
--group infection \
--override 'paths: output: ./infection/63ddd408b7cbd80f5c4ce426fcf9eeb4' \
--override 'coverage: enabled: false' \
--override 'bootstrap: ./infection/interceptor.codeception.63ddd408b7cbd80f5c4ce426fcf9eeb4.php' \
--override 'groups: infection: [/app/tests/integration/DatabaseTest.php]'

This approach would allow the current suite based config structure (or any other) and simplify path handling a lot (you may not even need it). Preparing mutation command line will be challenging but it looks doable - maybe a generic transformation of yaml to cli config values...

What do you think?

@dirx

This comment has been minimized.

Copy link

dirx commented Oct 21, 2019

@maks-rafalko ---override: extends: config.yml does not override already set config values (as expected... it extends...). This approach will not work. Overriding config values explicitly might still be an option, though.

@maks-rafalko

This comment has been minimized.

Copy link
Member Author

maks-rafalko commented Oct 21, 2019

Interesting idea ;) Didn't even think about it.

Unfortunately, after digging into existing issues of Codeception, I don't think it's possible.

As I understand, we are able to --override a setting in the main config, but it will be overridden by the nested suite config, which completely ruins the idea. Am I right?

Other things to note:

  1. we can have tens of files that cover mutated line of code
    • [minor] it can lead to a very long CLI command. According to SO, command line has a length limit, especially noticeable on Windows. Personally, I don't think this is an issue for us, just to mention
    • [minor] the long command line will pollute infection.log
  2. [?] Potentially, there can be issues with arguments escaping
  3. [minor] Not very convenient to maintain

Don't get me wrong. I'm not against this solution, just throwing my thoughts to discuss. If I'm wrong and it can work, I will be happy to implement it.

Rewriting paths seems to bring a lot of trouble.

Exactly. But this is not our fault, but how Codeception works:

  • Absolute URLs are not supported everywhere
  • Paths are calculated depending on the location of the main config (even if it is moved to /tmp by Infection)
  • Overcomplicated (IMO) config merge strategy.

If we had only 1 config with absolute paths, we would not have all these issues.

Personally, I would better require people, who want to use Infection+Codeception, to use only one codeception.yml global file in their projects, with suites configured there, rather than trying to support all the edge cases with merging, extending, overriding nested config files where the relative paths are calculated by not so clear rules.


@DavertMik I would really appreciate your input. If you have a chance, please read starting from this comment #800 (comment)

@Fenikkusu

This comment has been minimized.

Copy link
Contributor

Fenikkusu commented Oct 22, 2019

I would imagine it'd be pretty simple to add a new CLI option of --ignore-suite-config to codeception. Wouldn't that effectively take care of the issue? At that point, infection can be responsible for generating a combined suite config element and then just call codeception with the said flag? Perhaps I'm oversimplifying it, but that to me seems like the most viable option for most users.

One thing I am noticing is that when I was working on it, I added logic to recursively update the paths on all keys that start with .. I think you are doing it direct instead. As a result, you may not be updating the keys that are paths.

@Fenikkusu

This comment has been minimized.

Copy link
Contributor

Fenikkusu commented Oct 22, 2019

Better yet, I think i did ./. You could make that adjustment and then simply state all file paths must be prefixed ./.

@dirx

This comment has been minimized.

Copy link

dirx commented Oct 22, 2019

As I understand, we are able to --override a setting in the main config, but it will be overridden by the nested suite config, which completely ruins the idea. Am I right?

Is there any need to override suite based config values? I only see global config values (when there is no need to rewrite paths). According to current configuration builder those are:

  • Initial run
    • output path
    • enable coverage
    • coverage paths from infection config
    • shuffle tests
  • Mutation run
    • output path
    • disable coverage
    • groups config
    • bootstrap to intercept autoloader

As I understand suite configs can then only override suite based config values in global config. This won´t be an issue either.

Other things to note:

  1. we can have tens of files that cover mutated line of code

    • [minor] it can lead to a very long CLI command. According to SO, command line has a length limit, especially noticeable on Windows. Personally, I don't think this is an issue for us, just to mention
    • [minor] the long command line will pollute infection.log

The limited set of config values that need to be overriden plus using the param short form -o could help here.

  1. [?] Potentially, there can be issues with arguments escaping

This might need extra handling - escapeshellarg could do that.

  1. [minor] Not very convenient to maintain

True: it would work differently then for the other testframe adapters, but would be a slim solution with little (path rewriting) magic.

@maks-rafalko

This comment has been minimized.

Copy link
Member Author

maks-rafalko commented Oct 22, 2019

Is there any need to override suite based config values?

What about for example groups setting that can be declared on the suite level? As far as I understand, when we build a custom set of tests need to be executed for Mutant, providing --override 'groups: infection: [/path/to/test1.php, /path/to/test2.php], it can be overridden by the suites's groups setting.

Also, --override won't work when we pass a particular suite like codecept run acceptance -o '...' (currently not supported in this PR but definitely should be in the future, as we have the same feature for PHPUnit - passing --filter or --group options to run Infection only for a subset of the tests). Perhaps, we have to move forward this fix.

Anyway, let me give your idea a try in a separate branch since we all are not 100% sure on how Codeception works internally with merging configs and what issues we can face with. I will report back when I have news in the following couple of days.

@maks-rafalko

This comment has been minimized.

Copy link
Member Author

maks-rafalko commented Oct 30, 2019

Is there any need to override suite based config values?

Unfortunately, suite's config can override coverage setting, even when we pass --override 'coverage: enabled: false.

Example
# unit.suite.yml
...
# note that this value overrides *global* coverage setting even when it is located in the suite's config
coverage:
    example: true

command line:

vendor/bin/codecept run -vvv --override 'coverage: enabled: false'

Actual result inside Codeception code after merging / overriding:

[coverage] => Array
(
    [enabled] => true
)

Conclusion:

  • --override is being ignored for suite's config
  • not a blocker, just impacts performance of each Mutant

As I understand suite configs can then only override suite based config values in global config.

This is not true. For example groups, coverage and bootstrap are global root-level settings and they can be declared in the suite's config. Fortunately, groups are merged, not being overridden.


POC with --override options

I've pushed a new branch feature/codeception-override with the implementation of you idea @dirx with --override options. It's a POC so it has no tests, and in the very starting state at the moment.

I've also tested this branch with my test repositories as well as with your https://github.com/dirx/infection-codeception-test. And it works!

The only one change I made for your repo:

{
    "require-dev": {
        "php": "^7.3",
-        "infection/infection": "dev-feature/codeception",
+        "infection/infection": "dev-feature/codeception-override",
        "codeception/codeception": "^3.1"
    },

One annoying thing is that we can't simply do --override 'bootstrap: /path/to/custom/bootstrap.php because it is being merged after all classes are loaded, and our Interceptor does not work.

I had to also specify --bootstrap option to make it work. So the final command looks like:

'/app/vendor/codeception/codeception/codecept' 'run' '--no-colors' '--fail-fast' '--group' 'infection' '--bootstrap' '/app/./infection/interceptor.codeception.63ddd408b7cbd80f5c4ce426fcf9eeb4.php' '-o' 'paths: output: /app/./infection/63ddd408b7cbd80f5c4ce426fcf9eeb4' '-o' 'coverage: enabled: false'  '-o' 'groups: infection: [/app/tests/integration/DatabaseTest.php]'

@dirx, could you please look into it and probably try with other cases? Please note that some values are still hardcoded in that branch so it's not the final result.

Diff is here.

@dirx

This comment has been minimized.

Copy link

dirx commented Nov 2, 2019

@maks-rafalko Thank you for providing the override implementation. I did a quick test run with that feature branch on a smaller project with a simple unit & integration suite setup - in fact i did 3 runs with variations on --test-framework-options=unit,integration. No obstacles found - but I did not have time yet to look deeper into this. So looks very good on first sight.

@maks-rafalko maks-rafalko force-pushed the feature/codeception branch from 4560345 to cd57716 Nov 18, 2019
@maks-rafalko

This comment has been minimized.

Copy link
Member Author

maks-rafalko commented Nov 20, 2019

I completely rewrote implementation with the @dirx's idea and already merged it to this PR.

  • Codeception's integration is now simplier
  • It works for the cases where configuration is overridden in the ***.suite.yml
  • I've added @dirx's repository to our end-to-end test to make sure Infection can work with this tricky case

It is ready for review. I'm planning to merge it soon and release, don't want to freeze it anymore. So please, anyone interested - review.

@alfredbez

This comment has been minimized.

Copy link

alfredbez commented Nov 20, 2019

tests are green, not sure why Travis status is not in sync

Looks like the Deploy Step is not green, but canceled. Maybe it helps if you rerun the travis build

Copy link
Member

theofidry left a comment

I don't feel qualified to do give an approval since I'm no Codeception user. However the code looks good to me overall aside from a minor points.

It looks though that this could be split in two here: one PR which tweaks the framework adapter approach and another one taking advantage of the first to introduce the Codeception support.

.travis.yml Outdated
@@ -5,7 +5,7 @@ sudo: false
env:
global:
- INFECTION_FLAGS='--threads=4 --min-msi=67 --min-covered-msi=75 --coverage=coverage --log-verbosity=none'
- PHPUNIT_BIN='vendor/bin/phpunit --prepend=devTools/xdebug-filter.php --coverage-clover=clover.xml --coverage-xml=coverage/coverage-xml --log-junit=coverage/phpunit.junit.xml'
- PHPUNIT_BIN='vendor/bin/phpunit --prepend=devTools/xdebug-filter.php --coverage-clover=clover.xml --coverage-xml=coverage/coverage-xml --log-junit=coverage/junit.xml'

This comment has been minimized.

Copy link
@theofidry

theofidry Nov 20, 2019

Member

Could this change be done in a separate PR?

This comment has been minimized.

Copy link
@maks-rafalko

maks-rafalko Nov 23, 2019

Author Member

well, I extracted a couple of PRs from here. Others: benefits at this point of time, when it is already here and coupled to the changes in this PR

@@ -48,7 +48,7 @@ install:

test_script:
- cd c:\projects\workspace
- vendor\bin\phpunit --prepend=devTools/xdebug-filter.php --coverage-clover=clover.xml --coverage-xml=coverage/coverage-xml --log-junit=coverage/phpunit.junit.xml
- vendor\bin\phpunit --prepend=devTools/xdebug-filter.php --coverage-clover=clover.xml --coverage-xml=coverage/coverage-xml --log-junit=coverage/junit.xml

This comment has been minimized.

Copy link
@theofidry

theofidry Nov 20, 2019

Member

Could this change be done in a separate PR?

src/Command/InfectionCommand.php Outdated Show resolved Hide resolved
src/Mutant/MutantInterface.php Show resolved Hide resolved
src/TestFramework/CommandLineBuilder.php Outdated Show resolved Hide resolved
src/TestFramework/Factory.php Outdated Show resolved Hide resolved
src/TestFramework/Factory.php Outdated Show resolved Hide resolved
maks-rafalko added 24 commits Sep 4, 2019
…ling it each time we need to run test framework
… 3.1.0
… is provided by user
…d by speed
…implementation
… parameters required
…` file
…nd Mutation test runs
@maks-rafalko maks-rafalko force-pushed the feature/codeception branch from 2740989 to ae77bcb Nov 26, 2019
@maks-rafalko

This comment has been minimized.

Copy link
Member Author

maks-rafalko commented Nov 27, 2019

I think everyone who wanted to review this PR - did it, since it was opened 2 months ago.

Feel free though to comment, test and suggest anything regarding this PR. Since I'm not a Codeception user and don't have real projects - users who use Codeception can help a lot testing it.

I'm merging and planning to release it soon with Symfony 5 & PHP 7.4 support.

Thank you @dirx, @BackEndTea, @theofidry, @Fenikkusu

@maks-rafalko maks-rafalko merged commit d803a76 into master Nov 27, 2019
5 of 6 checks passed
5 of 6 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
PrettyCI Code formatting
Details
Scrutinizer Analysis: 2 new issues, 36 updated code elements – Tests: passed
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@theofidry theofidry deleted the feature/codeception branch Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.