galen icon indicating copy to clipboard operation
galen copied to clipboard

Auto generate dump on every checkLayout

Open gilligan opened this issue 8 years ago • 15 comments

I am currently working on a setup where I run check and dump during the same suite:

Regression ${pageName} for ${device} ${browser}
    selenium grid ${gridUrl} --page ${url}${path} --size ${size} --dc.build ${tunnelIdentifier} --dc.name "Galen Regression on ${device} ${browser}: ${pageName}" ${gridArgs} --dc.tunnelIdentifier ${tunnelIdentifier}
        run test/visual/scripts/${preAction}
        check test/visual/specs/page.regression.gspec --Vdevice "${device}" --VpageName ${pageName}
        dump test/visual/specs/page.dump.gspec --name "${pageName}-${device} Test" --export test-output/visual/dump/pages/${pageName}/${device}

This works: All checks are executed and a dump is created. However the json reporter only contains information on the check commands. It does not contain any information on dump commands.

When I remove the check I get a json report that contains info for the dump actions. I suppose it was never considered that check and dump would both be called? Or is it just some bug in the interface with the reporter?

gilligan avatar Jan 04 '17 10:01 gilligan

@ishubin could I possibly get any kind of feedback on that? That would be really great.

gilligan avatar Jan 17 '17 14:01 gilligan

Hi @gilligan! Sorry that it takes so long, I really have hard times maintaining all queries and forums. You are right, page dump was not designed to be part of test suite. It was supposed to work like a manual tool for helping writing specs or extracting images for elements. Then I realized that it could be useful to do that programmatically from the test suite and added wrappers for that. But as for the actual htmlreport, I didn't see a use case for page dump there. The only thing that I can think of that could be useful - is adding a report link to page.html from page dump. What is your use case? What exactly do you want to get in html report?

ishubin avatar Jan 17 '17 14:01 ishubin

@ishubin thank you for your reply. My workflow is somewhat involved. I want to be able to run dump and check at once. That does actually work. I get a report from the check which is also fine. My problem is that if either dump or check fails I am not able to tell which one it was.

The workflow I am trying to set up is one in which my CI triggers a job that does check and dump at once the branch. The branch is required to be on top of master. If the visual tests fail the developer needs to check why. If it fails because of expected differences then this is fine and the PR can be merged. Then the dump of that PR can be used to replace the master dump that all PRs compare against. I can literally just copy the files.

This is important to me because running our tests takes quite some time and our selenium grid resources are limited. Test execution can also sometimes just fail because of networking errors or flakyness etc. Being able to model my workflow in a way where I just copy over the PR dump over the master dump would thus be a huge benefit.

What is currently stopping me from that is that I cannot figure out if something fails because of check or because of dump.

Does that make any sense?

gilligan avatar Jan 17 '17 15:01 gilligan

@gilligan That actually makes a lot of sense to me now. Thanks for such a fascinating workflow description! I think we should incorporate something like this in galen framework. Though I am not sure yet how this is going to be implemented, but I do like the idea.

On another hand, what if you rewrite your tests in javascript? I think that way you can at least have your own error handlers and extend the galen core functionality with your own stuff. Like I did with my galen-bootstrap project

ishubin avatar Jan 17 '17 15:01 ishubin

@gilligan That actually makes a lot of sense to me now. Thanks for such a fascinating workflow descripI tion! I think we should incorporate something like this in galen framework. Though I am not sure yet how this is going to be implemented, but I do like the idea.

This would allow me to finally fix our broken workflow which currently includes a step in which I have a master branch that fails the regression test which then requires a new dump to go green again.

On another hand, what if you rewrite your tests in javascript? I think that way you can at least have your own error handlers and extend the galen core functionality with your own stuff. Like I did with my galen-bootstrap project

I wish we had gone for JS right away. The problem is that we now have quite some tests and converting them will require some effort. I still think this should ultimately happen though.

This being said I am now somewhat under pressure to find some way to realize this workflow. I would even accept to have to run a fork until this is properly incorporated into galen. Can you think of any way by which I could run check and dump in one go and and find out the success/failure of the dump afterwards? Even if it boils down to some hack like writing out a file somewhere.

Knowing that you like the general idea I would accept the technical debt of such some hack which could at some point be removed again.

gilligan avatar Jan 17 '17 16:01 gilligan

@gilligan, so let me get it straight. Correct me if I am wrong. You want to get the page dump automatically created after every check layout function, right? If so, then I don't think it would be that hard to implemented, but I can't promise you anything in terms of time due to my tight schedule.

ishubin avatar Jan 18 '17 13:01 ishubin

@ishubin what i would be perfectly fine with is to specify check and dump in one go as in my example in the issue description and then get either 2 report files: one for the dump and one for the check. Because now only one json report is written and it will only contain information on check and nothing about dump.

I need both in order to be able to tell if one or both failed if galen exits with != 0 exit code. Does that make any sense?

gilligan avatar Jan 18 '17 17:01 gilligan

Not really. I still don't get few things. For instance what do you mean by tell if one or both failed. I don't get how page dump could possible fail? If it fails to generate page dump - then it is an issue of galen-core and it needs to be fixed. There is nothing special about page dump generation so it should never ever fail. Also I just checked my generated json report and I see that for any layout check it already has a field objects which looks like this:

"objects" : {
   "menu.item-3" : {
     "area" : [ 212, 70, 100, 63 ]
   },
   "menu.item-4" : {
     "area" : [ 312, 70, 100, 63 ]
   },
   "login_button" : {
     "area" : [ 50, 373, 79, 45 ]
   },
   .....
}

In case you are writing your own parser for json report - you could already get that info. So then maybe I misunderstood your initial problem: is your issue that page dump is not executed at all and therefor no dump reports are generated?

ishubin avatar Jan 19 '17 09:01 ishubin

@ishubin oh I wish it was true that dumps could never fail. However we are running the dumps on a selenium grid (saucelabs) and believe me there are plenty reasons why things could go wrong there. Something just times out or there is some sort of flakyness so there is no guarantee that it will succeed.

That means if I get a non-zero exit code and the regression tests failed then I have no way of knowing if the dump also failed - or if it was just the check that found differences for example

gilligan avatar Jan 19 '17 11:01 gilligan

is your issue that page dump is not executed at all and therefor no dump reports are generated?

Ok let me try to be precise with this again. Lets assume I am using something like the following:

Regression ${pageName} for ${device} ${browser}
    selenium grid ${gridUrl} --page ${url}${path} --size ${size} --dc.build ${tunnelIdentifier} --dc.name "Galen Regression on ${device} ${browser}: ${pageName}" ${gridArgs} --dc.tunnelIdentifier ${tunnelIdentifier}
        run test/visual/scripts/${preAction}
        check test/visual/specs/page.regression.gspec --Vdevice "${device}" --VpageName ${pageName}
        dump test/visual/specs/page.dump.gspec --name "${pageName}-${device} Test" --export test-output/visual/dump/pages/${pageName}/${device}

I can get a json report which will contain info on the result of check. Also galen will exit with a non-zero exit code if something goes wrong. But if I get a non-zero exit code and I can find that there are errors in the json report for the check I still have no information whatsoever about what happened with dump. Did it succeed or not? There is no way of knowing (as far as I can tell)

gilligan avatar Jan 19 '17 11:01 gilligan

Ok, I think I got the full picture now. Now I see why page dump is so unstable. I have an idea but it will take some time since it requires quite some refactoring. The thing is this issue could actually be an extension of my other feature that I planned to implement anyway. So what I wanted to do is to optimize checkLayout to make it faster. So the idea is: while checking the layout galen would cache every element on page. Once the layout check is done it would have screenshot and all the elements already in memory. After this it would generate page dump and this time it should never fail as there will be no interaction with Selenium. By the way I have already a similar mechanism implemented for mutation testing in release-2.4 so I just need to properly refactor and use it.

ishubin avatar Jan 20 '17 08:01 ishubin

@ishubin sorry that this got so complicated. Since I am still not sure if we are really on the same page with this I would like to take a moment to explain our workflow to you. That might help in determining what it actually is that I need:

  • We have a CI that builds all branches and teams create PRs to get changes to master
  • We have a <master dump> which is a visual dump of the current production state
  • The visual regression tests are executed for each PR and by default compare against <master dump>
  • IF the PR introduces a visual change someone needs to trigger a dump to get a <branch-x dump>
  • The next time the regression tests are run they will pick <branch-x dump> as reference instead of <master dump> and the tests should be green.
  • When a PR is merged to master the <branch-x dump> of that branch will be copied over the <master dump>

While all of this works I was trying to change this in a way such that a dump is created automatically at the time the regression tests are executed. That way everything could be fully automatic: The tests would always compare with the <master dump> but after reviewing differences developers&designers could decide that the changes are OK and no additional execution is required since the dump for the branch was already created when the tests ran.

I can almost do that. The problem is -like I said before- when I combine check and dump I don't know what the status of dump is - In other words: if there were differences in the regression test and galen returns exit code 1 because of that I don't know if the dump was successful or not.

So from what I understand: in order to get this process to work I would need to be able to get a report from the check and from dump. When there are visual changes this is OK in the workflow described above but I need the dump to be successful so it can replace the master dump.

gilligan avatar Jan 20 '17 14:01 gilligan

@gilligan I think we are on the same page. It's just that I was getting into technical details of this possible feature implementation on the galen-core side. To make it short - I am going to introduce a flag that would auto-generate dump each time you call checkLayout. And this time it will always pass, so there will be no need to verify its status in the reports. Although of course if a checkLayout crashes in the middle (e.g. due to selenium grid issues) - page dump would not be generated, but then you could just easily check the report and see that.

ishubin avatar Jan 25 '17 15:01 ishubin

@ishubin thanks a lot. Sorry that this went on for so long I just wanted to make sure I am explaining myself correctly.

I am going to introduce a flag that would auto-generate dump each time you call checkLayout. And this time it will always pass, so there will be no need to verify its status in the reports. Although of course if a checkLayout crashes in the middle (e.g. due to selenium grid issues) - page dump would not be generated, but then you could just easily check the report and see that.

That's great!

gilligan avatar Jan 26 '17 10:01 gilligan

Unfortunately I wont be able to implement this feature in release 2.4 as it appeared that the scope of the feature is pretty big and needs a lot of refactoring. The release 2.4 is already delayed for quite a long time and I simply don't have enough time to finish all the planned features

ishubin avatar Sep 29 '18 08:09 ishubin