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

Set Fixture::$persistFixtureData if PERSIST_FIXTURE_DATA env var is set #12672

Merged
merged 1 commit into from Apr 3, 2018

Conversation

diosmosis
Copy link
Member

Useful for quickly running system tests when you have to run them over and over again.

@diosmosis diosmosis added the c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. label Mar 29, 2018
@tsteur
Copy link
Member

tsteur commented Mar 29, 2018

👍

@diosmosis diosmosis added this to the 3.4.1 milestone Mar 29, 2018
@diosmosis diosmosis added the Needs Review PRs that need a code review label Mar 29, 2018
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

The changes breaks the current behavior as changes from outside of this class might get overridden. We can either change it the way I suggested in the comment, or we need to change the way UI tests are set up. With your changes the parameter --persist-fixture-data does not work anymore


private function initFromEnvVars()
{
$this->persistFixtureData = (bool)getenv(self::PERSIST_FIXTURE_DATA_ENV);
Copy link
Member

@sgiehl sgiehl Mar 29, 2018

Choose a reason for hiding this comment

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

I'd suggest to change that to $this->persistFixtureData = $this->persistFixtureData || (bool)getenv(self::PERSIST_FIXTURE_DATA_ENV); so is stays true if that was set from outside already

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 will go w/ this approach, nice catch!

@diosmosis diosmosis added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels Mar 30, 2018
@diosmosis diosmosis force-pushed the persist-fixture-data-system branch from 34fe874 to 8098ba3 Compare April 1, 2018 19:24
@diosmosis diosmosis added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Apr 1, 2018
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

--persists-fixture-data works correctly for UI tests again, so guess should be good to merge now.

@diosmosis diosmosis merged commit 1259286 into 3.x-dev Apr 3, 2018
@diosmosis diosmosis deleted the persist-fixture-data-system branch April 3, 2018 23:30
@mattab mattab added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label May 8, 2018
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants