@mnapoli opened this issue on July 21st 2015

Fixes #7726

FYI I placed the screenshots into a new directory called expected-screenshots instead of the existing expected-ui-screenshots. The reason for that is because the existing dir is a submodule, and handling conflicts/branches with such a change is very very painful. Maybe after merging we could rename the directory if anyone prefers so.

TODO: - [x] rebase - [x] update to v0.5.3 - [x] update developer documentation - [x] ask core contributors to install git-lfs - [x] update to v0.5.4 - [x] green tests - [x] merge https://github.com/piwik/travis-scripts/pull/1 and update the travis-script submodule to point to master - [x] remove the expected-ui-screenshots submodule

@mnapoli commented on August 3rd 2015

Current status: even though I updated the screenshots, the Travis build uses a previous version. This is very weird because the screenshots are correct (up to date) both locally and on GitHub, but the screenshots being used in the Travis build are incorrect (not up to date).

One possibility is that git lfs is somehow downloading an old version of the screenshots… The reason for that is still unknown (is it a problem with the git extension, the GitHub git-lfs server, etc…).

@mattab commented on August 10th 2015

Hey @mnapoli, was there any progress or maybe you found some information what was not working? It would be so great to use git-lfs soon!

@mnapoli commented on August 10th 2015

@mattab I was waiting for a new release, but I'll try to make it work today and see if I can get around this buggy behavior.

@mnapoli commented on August 10th 2015

I'm still trying to make the build work (git lfs installation issues now with the new version). I just discovered this other issue and we might need to wait for it to be fixed: https://github.com/github/git-lfs/issues/564 (may be fixed in v0.6.0)

I'll try to have a green build (to have this PR working at least).

@mnapoli commented on August 10th 2015

I'm still stuck at the same place: Travis downloads old versions of some screenshots… I think it's a bug with git-lfs but it could be the client or the server… (maybe https://github.com/github/git-lfs/issues/564)

I would give it another try when v0.6.0 is released.

@mattab commented on August 14th 2015

it looks like they fixed https://github.com/github/git-lfs/issues/564 - which maybe would unblock us.

@mnapoli do you know if maybe they have nightly builds or beta builds that we could test already?

@mnapoli commented on August 14th 2015

@mattab had a look last time but it requires compiling the whole thing and there are no instructions IIRC. I can check again next week.

@mattab commented on August 14th 2015

if we're lucky, compiling could be as easy as ./configure && make && make install ?

@mnapoli commented on August 14th 2015

Nope, no Makefile, they use Go and they have custom script for e.g. Debian builds.

@mnapoli commented on August 19th 2015

Tried with the current master (https://github.com/piwik/travis-scripts/commit/bfb563a84577224f17f56a67799116d1b2530acb) but it seems images downloaded by git-lfs are not even correct images unfortunately. The build shows all image comparison failing (https://travis-ci.org/piwik/piwik/jobs/76290929) with:

Processed screenshot does not match expected for ActionsDataTable_column_sorted.png (image magick error: compare: improper image header `/home/travis/build/piwik/piwik/tests/lib/screenshot-testing/../../../tests/UI/./expected-screenshots/ActionsDataTable_column_sorted.png' @ error/png.c/ReadPNGImage/3246.)

Not looking good for now.

@tsteur commented on October 27th 2015

Had a rough look at this and noticed there were Error trying to create local media directory in '/home/travis/build/piwik/piwik/.git/lfs/objects/d1/5f': mkdir /home/travis/build/piwik/piwik/.git/lfs/objects/d1: permission denied errors in the output of git lfs fetch and it said (262 of 262 files) 0 B / 21.95 MB.

I fixed this and it does not output any errors anymore but says (262 of 262 files) 19.87 MB / 21.95 MB so it still seems to not fetch all data for some reasons. Maybe this bug will be fixed in the next git-lfs version and would be just wait until there is a new version available

@mattab commented on October 27th 2015

Maybe this bug will be fixed in the next git-lfs version and would be just wait until there is a new version available

Maybe we could check if it's a known bug and if not we could report it to git-lfs team?

@tsteur commented on October 27th 2015

I did and there are some reports and I think there's one PR in review that might fix it but could be also totally unrelated. I'd say for now easiest would be maybe just to wait for a little

@mattab commented on November 24th 2015

Moving to 3.0.0-b1 so we give it again another try

@sgiehl commented on August 4th 2016

Current status: UI tests are running now. See https://travis-ci.org/piwik/piwik/jobs/149604075 Need to update the expected files and clean up the code. Hopefully everything works then as expected :)

@tsteur commented on August 4th 2016

👍 👍

@sgiehl commented on August 4th 2016

Wondering if it is a good decision to move the screentshots from expected-ui-screenshots to expected-screenshots? That would make all plugins still using expected-ui-screenshots to have failing UI tests.

Otherwise it might cause problems when removing a submodule and adding a new directory with content at the same place. Might maybe problematic when rebasing other branches later...

@tsteur @mattab any thoughts on that?

@sgiehl commented on August 4th 2016

Build is green :tada: and files are stored in LFS (see for example https://github.com/piwik/piwik/blob/git-lfs/tests/UI/expected-screenshots/PeriodSelector_expanded.png)

So as soon as what I mentioned in the comment before is clarified we can merge this / use git lfs :+1:

@tsteur commented on August 8th 2016

I don't really have a preference. Ideally we would not break anything here, at least in Piwik 2. For Piwik 3 we could break it if needed. I think I also had sometimes problems with submodules that were afterwards directories and especially when switching between branches where eg one branch it is a submodule and another branch it is a directory. Maybe we could make it work to support both directories?

We would need to update the path in diff-viewer.js possibly, maybe change config.dist.js to allow a set of directories and I think this should not even be in the config as everyone who has a config.js will not receive the changes made there. So we could embed it into the code directly as it's not needed to change this anyway. Then in chai-extras.js we could search for a matching directory

A next step (but not in this PR) I am looking forward to would be moving all screenshots into the respective plugins apart from a few core ones like UIIntegration, Menus etc but this doesn't really help since we still need a directory under /tests/UI.

Awesome to see this work 🎉

@tsteur commented on August 14th 2016

do we maybe need to adjust the "sync screenshots command" to then check in which directory the screenshot should go to? Maybe we could also check in plugin directories eg plugins/*/tests/UI/expected.... This has been always a problem that it wasn't copying these files correctly. Now that they could be in different directories it may be worth to check in which directory a file currently exists and to move it there. If it exists in two folders which is very unlikely we would just replace the file in all folders maybe.

@sgiehl commented on August 14th 2016

I've updated the sync command to use the directory available. It doesn't replace in more than one path, as I don't think it's necessary.

@auchri commented on August 14th 2016

@sgiehl Typo: $possbileSubDirs

@mattab commented on August 16th 2016

Current status: UI tests are running now.

Wowza! well done @sgiehl building on top of @mnapoli - this will make all developers more efficient and happy :-)

@sgiehl commented on August 16th 2016

@mattab @tsteur should we merge that for 2.16.x already? so we can get rid of the screenshot submodule there, aswell? Otherwise I could also reopen this PR for 3.x

@mattab commented on August 16th 2016

I'm not sure if there is any problem with merging in 2.16 already, but it sounds good to me if it's possible. otherwise having Git lfs in 3.0.0 would definitely be good enough!

@tsteur commented on August 16th 2016

It may be save to do this in Piwik 2.x but ideally only after https://github.com/piwik/piwik/pull/10397 and only after we merged master into 3.X. To avoid huge problems

@sgiehl commented on August 16th 2016

I wouldn't see any problems in merging it to 2.x even before merging master into 3.x. There would be a merge conflict in the submodule for sure, and the screenshots needs to be updated for 3.x, but that shouldn't be a problem. But I'd be fine with the other way, as well.

@tsteur commented on August 19th 2016

OK in this case good to merge into 2.x 👍

@sgiehl commented on August 30th 2016

Are we going to merge that into 2.x? If so I would merge it to master this weekend and then merge master to 3.x, so it's included there, as well.

@tsteur commented on August 30th 2016

sounds good to me. FYI: I have merged master into 3.x just yesterday so it shouldn't be as painful :)

@sgiehl commented on August 30th 2016

Ok. I'll schedule this for friday night or saturday.

@tsteur commented on August 31st 2016

FYI: When you merge master => 3.x-dev I'd recommend to directly use the screenshots from https://github.com/piwik/piwik-ui-tests/tree/3.0-m06

@sgiehl commented on August 31st 2016

Already about to do that. Currently trying to resolve the merge conflicts...

@mnapoli commented on August 31st 2016

\o/ congrats!

This issue was closed on August 31st 2016
Powered by GitHub Issue Mirror