Navigation Menu

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

Fix issue when using includes in combination with override config #5978

Closed
wants to merge 1 commit into from

Conversation

bramstroker
Copy link

I am using include configs in my project to run multiple applications/modules in a single test run. https://codeception.com/docs/08-Customization#One-Runner-for-Multiple-Applications

In my root codeception.yml:

include:
    - src/OtherModule/*
paths:
    tests: tests
...

In my src/OtherModule I have another codeception.yml and seperate tests.
This is working perfectly and tests of the other module are also executed when I run vendor/bin codecept run.

However when using these feature the overriden configs (using -o parameter) are removed from the test run.
This was caused by line 365-368 which reset the complete config object to that of the main codeception.yml, but don't apply the override config again.
PHPStorm also uses this method to set a custom reporter -o "reporters: report: PhpStorm_Codeception_ReportPrinter", but because this was removed the complete UI reporting in PHPStorm was broken for me.

I also refactored to ONLY override the config when the test should be run by an include config. This seems less fragile to me.

@stormbyte
Copy link

+1

@bramstroker
Copy link
Author

Any update on this PR?

@DavertMik
Copy link
Member

Looks very good to me. If no objections we can merge this and release a new version

@DavertMik
Copy link
Member

@bramstroker is it possible to set up a test for it?
probably that will require to create a new test preset in https://github.com/Codeception/Codeception/tree/4.1/tests/data
like this one https://github.com/Codeception/Codeception/tree/4.1/tests/data/included

and create a test in "cli" suite to run tests using -o parameter to prove that parameters substitute correctly

@bramstroker
Copy link
Author

@DavertMik thanks. yes I can have a look into this somewhere next week.

@Naktibalda
Copy link
Member

@bramstroker I am waiting for test.

@bramstroker
Copy link
Author

I'm having a look into it now.

@bramstroker
Copy link
Author

bramstroker commented Oct 30, 2020

@DavertMik @Naktibalda
I want to provide a custom phpunit reporter within the test suite so I can use this reporter in my test.
This way I can check if the reporter can be overridden using the -o parameter and can check in the shell output if the custom reporter was executed.
However I am not sure what the right location is for this custom reporter so PSR-4 autoloading would work correctly.
I also see a custom report printer in tests/data/claypit/tests/_data/MyReportPrinter.php, and this claypit location is in composer autoload definitions. Not sure what claypit is however and if I need to put my report printer in the same location.
Could you provide some insights?

@Naktibalda
Copy link
Member

tests/data/claypit directory is copied to tests/data/sandbox before running each test in Cli test suite and tests are executed against sandbox directory because many tests generate code.
https://github.com/Codeception/Codeception/blob/4.1/tests/support/CliHelper.php#L17

Yes, you can put your custom printer to tests/data/claypit/tests/_data directory.

@calvinalkan
Copy link
Contributor

Is anybody still working on this?

What's missing to merge this?

This is a major pain point in a multi-application setup.

@Naktibalda
Copy link
Member

Custom reporters won't be supported in Codeception, they must be converted to extensions and used with --ext Name.
Use existing reporters as examples.
https://github.com/Codeception/Codeception/tree/5.0/src/Codeception/Reporter
https://github.com/Codeception/Codeception/blob/5.0/ext/DotReporter.php
https://github.com/Codeception/Codeception/blob/5.0/ext/SimpleReporter.php

@calvinalkan Do you have an issue with custom reporters or some other setting?

@calvinalkan
Copy link
Contributor

calvinalkan commented May 4, 2022

I have found that no matter what settings you put into the "root apps" configuration file, it has zero effects on included apps when you run it as a multi-app setup.

Neither options passed as command-line arguments nor options in the root codeception.yml file.

This causes the described PHPStrom UI bug here. But it's not exclusive to reporters.

I find this a bit counterintuitive:

I would expect it to work like this:

  • Anything that you pass via command-line or in the root config has priority over the hardcoded default config in Configuration.php.
  • The config of included apps has priority over the root config.

But how it currently works is:

  • Root config and command-line arguments are ignored
  • included app config is merged with hardcoded default config.

@Naktibalda Note, Im using 4.3.X. We currently cant upgrade but are tracking this at wp-browser: lucatume/wp-browser#571

@DavertMik
Copy link
Member

@calvinalkan

What's missing to merge this?

A test. Could you add one?

I would expect it to work like this:

Anything that you pass via command-line or in the root config has priority over the hardcoded default config in Configuration.php.
The config of included apps has priority over the root config.

I agree, please improve that behavior and we will merge it

@calvinalkan
Copy link
Contributor

@calvinalkan

What's missing to merge this?

A test. Could you add one?

I would expect it to work like this:

Anything that you pass via command-line or in the root config has priority over the hardcoded default config in Configuration.php.
The config of included apps has priority over the root config.

I agree, please improve that behavior and we will merge it

I can try, the static Configuration class is a bit tricky to navigate.

@calvinalkan
Copy link
Contributor

@DavertMik I fixed this including a test in #6452.

@Naktibalda
Copy link
Member

Replaced by #6452 , released as 4.2.0

@Naktibalda Naktibalda closed this Jul 28, 2022
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.

None yet

5 participants