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.
- [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
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…).
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!
@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.
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).
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.
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?
@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.
if we're lucky, compiling could be as easy as
./configure && make && make install ?
Nope, no Makefile, they use Go and they have custom script for e.g. Debian builds.
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.
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
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?
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
Moving to 3.0.0-b1 so we give it again another try
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 :)
Wondering if it is a good decision to move the screentshots from
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?
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:
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
Awesome to see this work 🎉
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.
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.
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 :-)
@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
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!
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
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.
OK in this case good to merge into 2.x 👍
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.
sounds good to me. FYI: I have merged master into 3.x just yesterday so it shouldn't be as painful :)
Ok. I'll schedule this for friday night or saturday.
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
Already about to do that. Currently trying to resolve the merge conflicts...