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

Omnifixture Update for automated UI screenshot tests #9272

Merged
merged 15 commits into from Jan 14, 2016
Merged

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Nov 24, 2015

Omnifixture wasn't update for a while now, so there are a lot of changes in UI tests that needs to be checked. Could anyone (@mattab, @tsteur, @diosmosis) please check the changed UI screenshots and mark the changes as correct or not.

@sgiehl sgiehl added Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Needs Review PRs that need a code review labels Nov 24, 2015
@sgiehl sgiehl added this to the 2.15.1 milestone Nov 24, 2015
@sgiehl
Copy link
Member Author

sgiehl commented Nov 24, 2015

Note: The rowevolution screenshots need #9241 to be merged in order to be processed correct. I'll do a rebase after that is done.

@diosmosis
Copy link
Member

My approach to updating OmniFixture is to ignore changes that are just to data. It would be near impossible to isolate any data inaccuracies in the UI tests, IMO it's better to rely on system tests for that. If the new data triggers UI bugs (ie, css issues, double encoding, xss issues, etc.), then it should be fixed, but otherwise trying to check if the data is accurate would only be waste of time.

@mattab
Copy link
Member

mattab commented Nov 25, 2015

I clicked like crazy through all of them to check. In general there is less data in the new omnifixture, which seems maybe un-expected?

examples:

BUG:

UNRELATED:

@mattab
Copy link
Member

mattab commented Nov 27, 2015

maybe you have some idea why the omnifixture now has less data @sgiehl @diosmosis ?

@sgiehl sgiehl force-pushed the omnifixtureupdate branch 2 times, most recently from 121c13c to 8e993b4 Compare December 14, 2015 22:59
@sgiehl sgiehl force-pushed the omnifixtureupdate branch 4 times, most recently from 1a71d15 to ea80bac Compare December 24, 2015 14:17
@sgiehl sgiehl force-pushed the omnifixtureupdate branch 2 times, most recently from 371d5bb to 64b0244 Compare December 24, 2015 23:10
@sgiehl
Copy link
Member Author

sgiehl commented Dec 25, 2015

I've tested for a while where the data differences are coming from.
It seems that after building the OmniFixture the archives included do not contain all data, so I used console commands to invalidate all archives and rebuilt them using core:archive. The resulting sql dump seems to contain nearly the data as before. See http://builds-artifacts.piwik.org/piwik/piwik/omnifixtureupdate/17335/

Does anyone know if there were any changes in the past few months that could cause the OmniFixture not to contain full archives?

@diosmosis
Copy link
Member

@sgiehl, @mattab asked me to work on this. Are you going to continue working on this? If so, please clarify with Matt.

@sgiehl
Copy link
Member Author

sgiehl commented Dec 25, 2015 via email

@diosmosis
Copy link
Member

I've rebased my local changes w/ your remote changes, so they're included for me locally.

@diosmosis
Copy link
Member

@mattab Latest build may pass, you can see the screenshot changes here: https://github.com/piwik/piwik-ui-tests/pull/5/files

@mattab
Copy link
Member

mattab commented Jan 11, 2016

@diosmosis Awesome! 👍 Could you please resolve conflict and merge both PR?

diosmosis added 9 commits January 13, 2016 04:42
…nger needed so when fixtures are setup together, the change won't carry over into other fixtures.
…adRealTranslations=1 is needed for archiving) so results are accurate. Also, do not use Fixture::getTestEnvironment() in fixtures, instead create new instances of TestingEnvironmentVariables so changes made to the file are not ignored/lost.
…r initial values before setting up each initial fixture. Ensures that changes to site properties by fixtures are reset before setting up the next fixture.
… so excessive sanitization is used and site name is stored properly in OmniFixture DB.
mattab pushed a commit that referenced this pull request Jan 14, 2016
@mattab mattab merged commit 15c2de7 into master Jan 14, 2016
@mattab mattab deleted the omnifixtureupdate branch January 14, 2016 00:04
@mattab mattab added c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. and removed Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. labels Jan 30, 2016
@mattab mattab changed the title Omnifixture Update Omnifixture Update for automated UI tests Jan 30, 2016
@mattab mattab changed the title Omnifixture Update for automated UI tests Omnifixture Update for automated UI screenshot tests Jan 30, 2016
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants