Enhanced screenshots to help understand flakiness

[François Guillot]

Hi,

We are using Geb, associated to a ChromeDriver instance to execute a bunch of browser tests on our application.

Some of those tests are flaky, mainly due to the lack of 'waitFor {}' calls around animations that happen when clicking a web element.

In order to help us understanding and fixing the flakiness, we capture screenshots when a test fails.

But this is sometimes not enough.

Say, the test fail with 'waitFor { someElement.displayed }', the screenshot will either show

- that the element is indeed not displayed

- that the element is displayed, but that doesn't help. It might be that by the time the screenshot is captured, the animation is actually finished and the screenshot doesn't really reflect the _exact_ state when the failure occurred.

So, in order to assist even more, I had the idea to enhance the screenshots with some data.

Basically, I added a 'ReportingListener', that grabs the created PNG screenshot, and writes a bunch of stuff on top if it, using java's Graphics2D API.

Currently, I add info like:

- the browser dimension (it can help in case of tooltips being displayed and hiding elements between them. These tooltips positions can differ based on the browser's dimension)

- the browser URL

- the current 'document.activeElement'

- the current cursor position.

  - since we can't get the mouse position directly, but only when the mouse moves (correct me if I'm wrong), I hack around this by adding a 'document.onmousemove = event => { window.mouseX = event.pageX; window.mouseY = event.pageY' script, and then do 'browser.interact { moveByOffset(1, 0) }', and then execute 'return [window.mouseX, window.mouseY]', and substract 1 to the mouseX returned value.

- I also capture the cursor position before and after click

  - I added a NavigatorEventListener

  - implement 'beforeClick' / 'afterClick' and use the same approach as above to store the Navigator.x / Navigator.y values in some variables to be picked up by the 'ReportingListener'

  - This helps a lot in case where clicking an element triggers an animation that moves the element being clicked.

     See for instance this page, and click the magnifying glass.

     In theory, if our 'waitFor { }' were perfect, the 'afterClick' cursor location should still be on the magnifying glass (that has moved vertically after the 'slide down' transition). But the screenshots now show that this is not the case, bringing an actual proof that the waits are not properly working. They just don't properly wait for the end of the animation.

This can be seen in the two attached screenshots :

- the cursor is correctly on the top-left corner of the magnifying glass on the 'beforeClick' image

- in the 'afterClick' one, the cursor has slightly moved vertically, following the slide down transition, but it actually is not on the top-left corner of the magnifying glass

Does this sound like a sensible approach to you?

Would it make sense that I work on making a PR to integrate that in some sort of ways in Geb?

[François Guillot]

I actually forgot to add the screenshots mentioned...

[Marcin Erdmann]

Hi François,

Thank you very much for your email. It's great to learn that Geb is being used to test the frontend of Gradle Enterprise. It's also cool to see that a fairly new addition to Geb in the form of NavigatorEventListener is being used out there in the wild.

I think what you're doing makes a lot of sense. I think most folks writing browser tests for modern, single page apps deal with flakiness caused by async events in one form or another. For example, on my current project, we recently had a push to quash flakiness in our tests and introduced a number of custom reporters which turned out to be very effective:
- one which writes browser console logs

- one which remote controls (https://github.com/ldaley/remote-control) into the server and writes a thread dump to see if any long running operations are currently in flight

Both of them could probably be pulled into Geb core in one form or another, with the biggest stumbling block probably being making the first one cross browser.

I find big parts of what you've done useful and generic enough to be pulled into core. Let me comment one by one:

1. I don't see much value in adding the window size onto the screenshot. I personally tend to make my tests reliable with regards to browser size by always setting the browser size to a predefined value when constructing a driver instance, e.g.:

driver.manage().window().size = new Dimension(1024, 768)

2. For reporting browser url I would probably introduce a new geb.report.Reporter implementation that writes the url into a text file, I don't think that there is a valid reason for why that information should be written onto the screenshot

3. Writing out current active element and mouse position looks great. We could enhance geb.report.ScreenshotReporter with configuration properties like reportCurrentElement and reportMousePosition and write that info into the screenshot if these properties are set - that way it would be a backwards compatible change and we could change the default values of these props to true in the next major version. Unfortunately I don't have the capacity at the moment to see if one can get mouse position in a cleaner or more efficient way than you have done.

4. For dealing with rendering the position of what will be clicked I think we should extend the reporting mechanism to be able to pass additional information via some kind of a context and then react in the reporter to that data being set and perform additional actions.

If you’re up for contributing back what you have then I’d suggest attacking points 2 and 3 and when we get that merged I will tackle 4 cause that will be a bit more involved with regards to introducing a reporting context.

Cheers,

Marcin

--
You received this message because you are subscribed to the Google Groups "Geb User Mailing List" group.
To unsubscribe from this group and stop receiving emails from it, send an email to geb-user+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/geb-user/4042ad52-757e-41b9-be2a-9bee2639623e%40googlegroups.com.

[François Guillot]

Hi Marcin

A few comments inline..

On Saturday, February 8, 2020 at 1:20:29 PM UTC+1, Marcin Erdmann wrote:

Hi François,

Thank you very much for your email. It's great to learn that Geb is being used to test the frontend of Gradle Enterprise. It's also cool to see that a fairly new addition to Geb in the form of NavigatorEventListener is being used out there in the wild.

With Luke on the team, what would you expect ;) ? We've been using it right from the start.

 

I think what you're doing makes a lot of sense. I think most folks writing browser tests for modern, single page apps deal with flakiness caused by async events in one form or another. For example, on my current project, we recently had a push to quash flakiness in our tests and introduced a number of custom reporters which turned out to be very effective:
- one which writes browser console logs

- one which remote controls (https://github.com/ldaley/remote-control) into the server and writes a thread dump to see if any long running operations are currently in flight

Both of them could probably be pulled into Geb core in one form or another, with the biggest stumbling block probably being making the first one cross browser.

I find big parts of what you've done useful and generic enough to be pulled into core. Let me comment one by one:

1. I don't see much value in adding the window size onto the screenshot. I personally tend to make my tests reliable with regards to browser size by always setting the browser size to a predefined value when constructing a driver instance, e.g.:

driver.manage().window().size = new Dimension(1024, 768)

We do have such setting, but with large width/height.
It seems that when executed locally, if the window dimension is larger than your machine screen, then the dimension is capped to the screen. Therefore we sometimes have different behaviors locally on and CI (e.g. tooltip hiding an element that is then not clickable, due to the positioning of the tooltip being somewhat influenced by the window dimension).

 

2. For reporting browser url I would probably introduce a new geb.report.Reporter implementation that writes the url into a text file, I don't think that there is a valid reason for why that information should be written onto the screenshot

Agreed. This was a quick hack (yet valuable) to pinpoint some missing waitFor's.

That being said, having to look at a text file _and_ a screenshot is less ideal to get the global 'context' of a browser test failure.

3. Writing out current active element and mouse position looks great. We could enhance geb.report.ScreenshotReporter with configuration properties like reportCurrentElement and reportMousePosition and write that info into the screenshot if these properties are set - that way it would be a backwards compatible change and we could change the default values of these props to true in the next major version. Unfortunately I don't have the capacity at the moment to see if one can get mouse position in a cleaner or more efficient way than you have done.

4. For dealing with rendering the position of what will be clicked I think we should extend the reporting mechanism to be able to pass additional information via some kind of a context and then react in the reporter to that data being set and perform additional actions.

If you’re up for contributing back what you have then I’d suggest attacking points 2 and 3 and when we get that merged I will tackle 4 cause that will be a bit more involved with regards to introducing a reporting context.

I'll try to prepare something on my time off. It's therefore going to take a bit of time, but I'll do it.

While we're discussing improvements, it would be great than the 'context' you're referring to was shared between the beforeClick / afterClick callbacks., and if we could somehow inject an arbitrary object into it.

The use case is that we have some clicks that trigger an animation (using velocity.js). The targeted element has a 'velocity-animating' CSS class when being animated. We currently use the presence (or the lack thereof) of this CSS class to ensure the animation is fully done. But the logic is not perfect, because if we're entering the 'afterClick' callback before the animation has even started (which could happen when the browser is under stress, like in CI), we don't find the 'velocity-animating- CSS class, and think the animation is over. To overcome this, I still need to have a little waitFor with a timeout, to ensure sufficient time is waited (for the likeliness of this scenario to happen to be near zero)

If we share an object between the beforeClick/afterClick context, I can then register something that somehow is notified when the animation will start (and we know it will start), and then the afterClick can query this same object to know if the animation has started (and potentially is already over) reliably.

 

Cheers,

Marcin

To unsubscribe from this group and stop receiving emails from it, send an email to geb-...@googlegroups.com.

[Marcin Erdmann]

Hi François,

Apologies for the delay in my response, I was on holidays.

On Sat, Feb 8, 2020 at 1:34 PM François Guillot <fran...@gradle.com> wrote:

With Luke on the team, what would you expect ;) ? We've been using it right from the start.

You never know, people move on and find new toys to play with. :)

 

We do have such setting, but with large width/height.
It seems that when executed locally, if the window dimension is larger than your machine screen, then the dimension is capped to the screen. Therefore we sometimes have different behaviors locally on and CI (e.g. tooltip hiding an element that is then not clickable, due to the positioning of the tooltip being somewhat influenced by the window dimension).

Right, I see.

 

2. For reporting browser url I would probably introduce a new geb.report.Reporter implementation that writes the url into a text file, I don't think that there is a valid reason for why that information should be written onto the screenshot

Agreed. This was a quick hack (yet valuable) to pinpoint some missing waitFor's.

That being said, having to look at a text file _and_ a screenshot is less ideal to get the global 'context' of a browser test failure.

I agree with you that it's more convenient to have that info all in one file, but we have to be careful when considering pulling stuff into core not to make the screenshots a dumping ground for information that might be useful to some users. As I said, I'd be more than happy to accept a PR with a custom reporter which reports on the url and dimensions of the browser in a text form but I'm afraid I will have to say no to including that information in the screenshot as I personally believe it is not the right place for that information.

 

I'll try to prepare something on my time off. It's therefore going to take a bit of time, but I'll do it.

Looking forward to it. Fingers crossed that you will get round to it at some point, however long that might take.

 

While we're discussing improvements, it would be great than the 'context' you're referring to was shared between the beforeClick / afterClick callbacks., and if we could somehow inject an arbitrary object into it.

The use case is that we have some clicks that trigger an animation (using velocity.js). The targeted element has a 'velocity-animating' CSS class when being animated. We currently use the presence (or the lack thereof) of this CSS class to ensure the animation is fully done. But the logic is not perfect, because if we're entering the 'afterClick' callback before the animation has even started (which could happen when the browser is under stress, like in CI), we don't find the 'velocity-animating- CSS class, and think the animation is over. To overcome this, I still need to have a little waitFor with a timeout, to ensure sufficient time is waited (for the likeliness of this scenario to happen to be near zero)

If we share an object between the beforeClick/afterClick context, I can then register something that somehow is notified when the animation will start (and we know it will start), and then the afterClick can query this same object to know if the animation has started (and potentially is already over) reliably.

The context I talked about in my previous email was meant to be used to communicate between a caller to report() and various reporters. What you are talking about seems to be a completely different concern which has to do with communicating between respective before/after methods on NavigatorEventListener and possibly also between beforeAtCheck and onAtCheckSuccess/onAtCheckFailure methods on PageEventListener. This sounds like it might be something useful - please feel free to create an issue for implementing this in the tracker at https://github.com/geb/issues/issues. 

On the other hand I'm not sure I understand how this helps you to work around the fact that there is a period of time between something being clicked and the animation marker class being added to the animated element but maybe I don't fully grok what you're trying to explain or do not have the full context to be able to do so. In my opinion one cannot reliably wait for an asynchronously initiated (the fact that it's asynchronous being the key here), temporary state to complete because you either run into the issue that you will consider the state to be completed because you check for it before it even started or you run into the possibility of not detecting the state (animation) to occur at all if it is very short and it manages to start and stop between your polling checks.

Marcin

[François Guillot]

Le dim. 16 févr. 2020 à 14:58, Marcin Erdmann <marcin....@proxerd.pl> a écrit :

Hi François,

Apologies for the delay in my response, I was on holidays.

On Sat, Feb 8, 2020 at 1:34 PM François Guillot <fran...@gradle.com> wrote:

With Luke on the team, what would you expect ;) ? We've been using it right from the start.

You never know, people move on and find new toys to play with. :)

 

We do have such setting, but with large width/height.
It seems that when executed locally, if the window dimension is larger than your machine screen, then the dimension is capped to the screen. Therefore we sometimes have different behaviors locally on and CI (e.g. tooltip hiding an element that is then not clickable, due to the positioning of the tooltip being somewhat influenced by the window dimension).

Right, I see.

We might decide to cap the browser dimension to a dimension lower than a standard 15" laptop screen though, to get around the problem. 

 

2. For reporting browser url I would probably introduce a new geb.report.Reporter implementation that writes the url into a text file, I don't think that there is a valid reason for why that information should be written onto the screenshot

Agreed. This was a quick hack (yet valuable) to pinpoint some missing waitFor's.

That being said, having to look at a text file _and_ a screenshot is less ideal to get the global 'context' of a browser test failure.

I agree with you that it's more convenient to have that info all in one file, but we have to be careful when considering pulling stuff into core not to make the screenshots a dumping ground for information that might be useful to some users. As I said, I'd be more than happy to accept a PR with a custom reporter which reports on the url and dimensions of the browser in a text form but I'm afraid I will have to say no to including that information in the screenshot as I personally believe it is not the right place for that information.

Totally agree with you here. If it ends up in core, it will be on a separate report. 

 

I'll try to prepare something on my time off. It's therefore going to take a bit of time, but I'll do it.

Looking forward to it. Fingers crossed that you will get round to it at some point, however long that might take.

 

While we're discussing improvements, it would be great than the 'context' you're referring to was shared between the beforeClick / afterClick callbacks., and if we could somehow inject an arbitrary object into it.

The use case is that we have some clicks that trigger an animation (using velocity.js). The targeted element has a 'velocity-animating' CSS class when being animated. We currently use the presence (or the lack thereof) of this CSS class to ensure the animation is fully done. But the logic is not perfect, because if we're entering the 'afterClick' callback before the animation has even started (which could happen when the browser is under stress, like in CI), we don't find the 'velocity-animating- CSS class, and think the animation is over. To overcome this, I still need to have a little waitFor with a timeout, to ensure sufficient time is waited (for the likeliness of this scenario to happen to be near zero)

If we share an object between the beforeClick/afterClick context, I can then register something that somehow is notified when the animation will start (and we know it will start), and then the afterClick can query this same object to know if the animation has started (and potentially is already over) reliably.

The context I talked about in my previous email was meant to be used to communicate between a caller to report() and various reporters. What you are talking about seems to be a completely different concern which has to do with communicating between respective before/after methods on NavigatorEventListener and possibly also between beforeAtCheck and onAtCheckSuccess/onAtCheckFailure methods on PageEventListener. This sounds like it might be something useful - please feel free to create an issue for implementing this in the tracker at https://github.com/geb/issues/issues. 

Yes that is exactly that. 

On the other hand I'm not sure I understand how this helps you to work around the fact that there is a period of time between something being clicked and the animation marker class being added to the animated element but maybe I don't fully grok what you're trying to explain or do not have the full context to be able to do so. In my opinion one cannot reliably wait for an asynchronously initiated (the fact that it's asynchronous being the key here), temporary state to complete because you either run into the issue that you will consider the state to be completed because you check for it before it even started or you run into the possibility of not detecting the state (animation) to occur at all if it is very short and it manages to start and stop between your polling checks.

It's just that if you share some state between the before and after click, you're sure to not miss the start of the animation, and then poll for its end. 

You're right that this cannot work based on polling only, the state holder would have to be notified when the animation start, but I'm sure there must be some ways to achieve that, no? 

Marcin

--
You received this message because you are subscribed to the Google Groups "Geb User Mailing List" group.

To unsubscribe from this group and stop receiving emails from it, send an email to geb-user+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/geb-user/CA%2B52dQRCK%3DodyUhS-oqpy-5sBdf9kDiQBOakmPg8eo5a8oW7tw%40mail.gmail.com.

[Marcin Erdmann]

On Sun, Feb 16, 2020 at 2:32 PM François Guillot <fran...@gradle.com> wrote:

I agree with you that it's more convenient to have that info all in one file, but we have to be careful when considering pulling stuff into core not to make the screenshots a dumping ground for information that might be useful to some users. As I said, I'd be more than happy to accept a PR with a custom reporter which reports on the url and dimensions of the browser in a text form but I'm afraid I will have to say no to including that information in the screenshot as I personally believe it is not the right place for that information.

Totally agree with you here. If it ends up in core, it will be on a separate report. 

Awesome.

  

The context I talked about in my previous email was meant to be used to communicate between a caller to report() and various reporters. What you are talking about seems to be a completely different concern which has to do with communicating between respective before/after methods on NavigatorEventListener and possibly also between beforeAtCheck and onAtCheckSuccess/onAtCheckFailure methods on PageEventListener. This sounds like it might be something useful - please feel free to create an issue for implementing this in the tracker at https://github.com/geb/issues/issues. 

Yes that is exactly that. 

On the other hand I'm not sure I understand how this helps you to work around the fact that there is a period of time between something being clicked and the animation marker class being added to the animated element but maybe I don't fully grok what you're trying to explain or do not have the full context to be able to do so. In my opinion one cannot reliably wait for an asynchronously initiated (the fact that it's asynchronous being the key here), temporary state to complete because you either run into the issue that you will consider the state to be completed because you check for it before it even started or you run into the possibility of not detecting the state (animation) to occur at all if it is very short and it manages to start and stop between your polling checks.

It's just that if you share some state between the before and after click, you're sure to not miss the start of the animation, and then poll for its end. 

You're right that this cannot work based on polling only, the state holder would have to be notified when the animation start, but I'm sure there must be some ways to achieve that, no? 

I'm sorry but I don't see a way to achieve that... How would you register anything to be triggered when the animation starts? Does velocity.js have a callback for that? Even if it does, then it will be executed in the browser and as far as I know there is no way to call back from javascript to Selenium. It feels to me that what you're after is adding a class that signifies that an element is being animated in an on click handler in js which will be synchronous and then removing that class when an animation completes (looks like velocity.js allows to register a function which is executed when an animation completes). The below trait shows what I propose applied to css transitions and based on transitionend event (https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/transitionend_event):

trait CssTransitionWaitingSupport implements Navigator, WaitingSupport {

    abstract JavascriptInterface getJs()

    void waitForCssTransitionToFinish(Closure trigger) {
        js.exec(singleElement(), '''
            var o = jQuery(arguments[0]);
            window.setTransitionFinishedClass = function() {
                $(this).addClass('transitionFinished');
            }
            o.bind('transitionend', window.setTransitionFinishedClass);
        ''')
        try {
            trigger.call()
            waitFor { hasClass('transitionFinished') }
        } finally {
            js.exec(singleElement(), '''
                var o = jQuery(arguments[0]);
                o.unbind('transitionend', window.setTransitionFinishedClass);
                o.removeClass('transitionFinished')
                window.setTransitionFinishedClass = undefined;
            ''')
        }
    }
}