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

Update Omnifixture and add test fixture #9895

Closed
wants to merge 0 commits into from

Conversation

andrzejewsky
Copy link

Recently, I made small PR: #9863 - it was merged without any tests.
I'd like to improve it eg. in order to detect some regression in the future, thus I created this PR.

What do you think about it? @piwik/core-team?

Furthermore, here is PR for updating screenshots: https://github.com/piwik/piwik-ui-tests/pull/6
Look at the visitor log at the last entry: https://github.com/piwik/piwik-ui-tests/blob/updateScreenshots/UIIntegrationTest_visitors_with_site_search_visitorlog.png (before, it was displayed incorrectly)

@andrzejewsky andrzejewsky changed the title Missing test for #9863 Missing test for https://github.com/piwik/piwik/pull/9863 Mar 8, 2016
@andrzejewsky andrzejewsky changed the title Missing test for https://github.com/piwik/piwik/pull/9863 Missing test for #9863 Mar 8, 2016
@mattab
Copy link
Member

mattab commented Mar 8, 2016

Hi @andrzejewsky

I'm not sure but maybe you have directly edited the DB dump to include the new test case?

The Omnifixture is generated by dumping the DB after importing all existing Fixtures in Piwik. When you want to modify the Omnifixture you need to:

Let me know what you get!

@andrzejewsky andrzejewsky added the Needs Review PRs that need a code review label Mar 8, 2016
@andrzejewsky
Copy link
Author

Ok @mattab I created SQL dump according to your suggestions. As you see there is a new fixture with site search string (which was displayed incorrectly)

@tsteur
Copy link
Member

tsteur commented Mar 9, 2016

BTW in case you're keen to do it: Feel free to change OmniFixture-dump.sql.gz to OmniFixture-dump.sql. For this just unpack the gz file, remove the gz here: https://github.com/piwik/piwik/blob/2.16.1-b1/tests/PHPUnit/Fixtures/UITestFixture.php#L36 . This should work and this way we get a diff for the changed OmniFixture in the future. There's no reason for it being a gz file apart from trying to save space. However, space is not such a big issue I would say and the file isn't changing so much.

@mattab mattab modified the milestone: 2.16.1 Mar 10, 2016
@mattab
Copy link
Member

mattab commented Mar 14, 2016

@andrzejewsky thanks for the PR. Could you next:

then we will merge 👍

@mattab mattab 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 14, 2016
@mattab mattab changed the title Missing test for #9863 Update Omnifixture and add test fixture Mar 15, 2016
@mattab
Copy link
Member

mattab commented Apr 7, 2016

@andrzejewsky I can't merge the PR I think due to the submodule conflict. Maybe you could leave the submodule as it is on master branch, ie. remove it from this PR? (I'll make sure to make the build green afterwards and update the submodule).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants