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

Test fixes from 4.4.2 to 5.x merge #11476

Merged
merged 4 commits into from
Sep 14, 2022
Merged

Conversation

escopecz
Copy link
Sponsor Member

Q A
Bug fix? (use the a.b branch) [Y]
New feature/enhancement? (use the a.x branch) [N]
Deprecations? [N]
BC breaks? (use the c.x branch) [N]
Automated tests included? [N]
Related user documentation PR URL /
Related developer documentation PR URL /
Issue(s) addressed Fixes #11402

Description:

#11196 added dynamic table prefix to the tests for a good reason. However, it means that the cache must be cleared before every test execution. A developer can run test(s) many times per day and building the cache takes tens of seconds. That means it's a big lose of time.

This PR changes it to static prefix which can be defined by .env(.dist) file or by defining env var in the phpunit command execution itself. There is small chance that the developer uses the same table prefix on local, production and test environment so the problem with hard-coded prefixes should be discovered in some environment.

We had problem in #11438 that the CI tests were still failing even after the static table prefix. So this PR also contains unified loading of the env file and setting MAUTIC_ENV=test in the CI which actually fixed the issue as after that the config_test.php was properly loaded.

Steps to test this PR:

  1. No testing is needed as no production code was changed. Just changes in how we run tests. So review the changes and check the CI that it is green and the output for PHPUNIT tests makes sense.

@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Sep 14, 2022
@escopecz escopecz added bug Issues or PR's relating to bugs code-review-needed PR's that require a code review before merging regression A bug that broke something in the last release automated-tests Anything related to unit, functional or e2e tests labels Sep 14, 2022
@escopecz escopecz added this to the 4.4.3 milestone Sep 14, 2022
@escopecz escopecz marked this pull request as ready for review September 14, 2022 08:24
@escopecz escopecz mentioned this pull request Sep 14, 2022
@codecov
Copy link

codecov bot commented Sep 14, 2022

Codecov Report

Merging #11476 (233e4e7) into 4.4 (dedf40b) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##                4.4   #11476   +/-   ##
=========================================
  Coverage     49.36%   49.37%           
  Complexity    35401    35401           
=========================================
  Files          2144     2144           
  Lines        105532   105532           
=========================================
+ Hits          52101    52102    +1     
+ Misses        53431    53430    -1     
Impacted Files Coverage Δ
...eBundle/EventListener/DoctrineEventsSubscriber.php 74.54% <0.00%> (+1.81%) ⬆️

Copy link
Contributor

@mollux mollux left a comment

Choose a reason for hiding this comment

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

Fix is good to solve the seen issue.

I still don't get how the MAUTIC_ENV change is needed, as

So even is there is logic that would boot mautic with the wrong env, it would only fill up the cache for the wrong env.
PHPUnit will alwas use the test env (unless defined otherwise, but this isn't the case)

I may dive deeper to really understand this issue, as we can't reproduce this on 4.x or local, and there is no substantial change in 5.x branch that can explain this.
It looks like there is an external factor (github action, ...) that results in this funky situation.

But as this solves this weird situation, I don't see any issue merging this.

@escopecz escopecz added the ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged label Sep 14, 2022
@escopecz escopecz merged commit acf3393 into mautic:4.4 Sep 14, 2022
@escopecz escopecz deleted the fixes-from-4.4.2-merge branch September 14, 2022 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automated-tests Anything related to unit, functional or e2e tests bug Issues or PR's relating to bugs cla-signed The PR contributors have signed the contributors agreement code-review-needed PR's that require a code review before merging ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged regression A bug that broke something in the last release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants