alphagov / govuk_frontend_toolkit

❗️GOV.UK Frontend Toolkit is deprecated, and will only receive major bug fixes and security patches.
MIT License
403 stars 107 forks source link

Broken Selenium test with new radio/checkbox styles #388

Closed esr360 closed 7 years ago

esr360 commented 7 years ago

We've spent 2 days trying to figure out why our Selenium tests suddenly stopped working when updating to the new radio/checkbox styles.

I thought I'd open this issue to potentially save others with the same problem, and hopefully discuss what the best solution is.

Previously the test looked for input elements with a specific ID in order to click them. The new styles are physically hiding the input element (with opacity) in favour of the pseudo checkbox/radio control, and this prevents the Selenium tests from being able to click them. We are currently looking at the best way to fix this without having to completely re-write our tests - if anyone has any insight it would be much appreciated.

Thanks

venkiteelay commented 7 years ago

Hi Ed,

Thanks for raising this.

robinwhittleton commented 7 years ago

When I upgraded Verify I had to add allow_label_click to get it to pass. That’s obviously a Capybara thing, but it’s possible to drive Selenium to do the same thing. Are you using a runner for Selenium?

esr360 commented 7 years ago

There are definitely ways to overcome the issue, but they all mostly involve re-writing the tests, for example to click the label instead of the input, which was something we were hoping to avoid, as we have a lot of radios and checkboxes.

esr360 commented 7 years ago

Something else worth noting is that if the label contains a link and we tell Selenium to click the label, this will trigger a click of the link, which may not be what we want, as in this case where we want to check its corresponding input, which is why we were originally clicking the inputs and not the labels in the first place.

esr360 commented 7 years ago

The creative way we came up with to quickly solve this problem in the short term was to inject a script when Selenium runs which finds all radios/checkboxes and sets their opacity to 1.

Closing the issue as it's not really a problem with the toolkit itself.

robinwhittleton commented 7 years ago

That’s what Digital Marketplace settled on in the end, and I believe DWP (via @adamsilver) have done the same.

I guess it might be worth experimenting in the future in setting the checkboxes / radios to opacity: 0.01 or something – low enough to not be visible in any situation but potentially enough for Selenium? Or document that as users are actually clicking on the labels that Selenium clicks should trigger a click event against the label too.

NickColley commented 7 years ago

@robinwhittleton that's not a bad idea, worth a try!

adamsilver commented 7 years ago

@robinwhittleton we are waiting for the no-javascript version before attempting to run that solution.

For me, a Selenium test should test how users actually use the UI. In this case they will be interacting with the label and so tests in fact should change to accommodate this behaviour.

Interestingly, clicking the label (even if the radio button or checkbox was a native one, and not hidden), should be tested anyway, and by not testing this (for any form control for that matter) is a gap in coverage. A small gap, but a gap nonetheless.