This issue is parallel to drupal core's
#2942900: Convert JavascriptTestBase Tests to use DrupalSelenium2Driver
What is actually on my mind is that this a first step to resolving a newly discovered test instability.
Recently to prove if we had a test instability I changed only code comments and observed that the test failures changed. :(
https://www.drupal.org/project/flag/issues/2894095#comment-12670926
Before I chase down the bug I want to update to the latest way of working ..
The last time we resolved some test instability, in the flag module, it was due in part because of differences between modern and deprecated versions of core behaving subtly different.
I hope this explains why I have graded a seemingly simple maintenance task as a critical bug....
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 2983268-16-interdiff.txt | 3.75 KB | berdir |
| #16 | 2983268-16.patch | 9.19 KB | berdir |
| #15 | interdiff-2983268-11-15.txt | 2.17 KB | martin107 |
| #15 | 2983268-15.patch | 7.52 KB | martin107 |
Comments
Comment #2
martin107 commentedThis patch also updates flag_bookmark.
Comment #3
martin107 commentedMy patch is a little premature, as #2942900: Convert JavascriptTestBase Tests to use DrupalSelenium2Driver is still RTBC
Comment #4
martin107 commentedThe eagle has landed.
Comment #5
socketwench commentedReeeeeeee----testing!
Comment #6
socketwench commentedI'm getting the same error. Does this only work on 8.6?
Comment #7
martin107 commentedAh yes, sorry forgot to mention that, my bad... 8.6 'cos #2942900: Convert JavascriptTestBase Tests to use DrupalSelenium2Driver
Comment #8
socketwench commentedHm. 8 failures even on 8.6.
Comment #9
martin107 commentedAh, sorry, I am having to juggle my time carefully ... at the moment...
I will try and find time to look at this ... soon.
Comment #10
martin107 commentedtriggering a test on D8.7
Comment #11
martin107 commentedIn the cold light of day - Most, but not all of the errors are trivial to see and fix.... one remains.
Comment #12
socketwench commentedRunning it through the test again since I wonder if we're still getting erratic failures.
Comment #13
socketwench commentedNope, doesn't look like that issue.
Comment #14
martin107 commentedThanks for looking at this.
My sense from local tests is that the failure is very repeatable - with all the good and bad that brings.
I need to carve out a little time over the weekend but my current theory is that things are now much faster in testing - and our waiting strategy needs to be tighter.
so the click button, wait for DOM sequence is muddled - the second AJAX command response is maybe not complete, or the DOM render is not finished... that is what is out of my grasp
I will try and sit down with a coffee on Saturday and get to the bottom of it.
Comment #15
martin107 commentedBefore I start... to my mind, I am open to the criticism that I haven't got to the bottom of the issue ... and than I am just playing with my food...
So went back to 'Question all the things' and one aspect unrelated to the test failures was ..
"Our test are ok but being paranoid our tests do not actually check that the message vanishes."
If the link is visible it should be clickable ... and our previous tests should be water tight!
In this patch I have inserted a delay while waiting for the message to go... and hey presto the test accidentally now pass locally...
I just want to see if this is fixed remotely/definitely.
Comment #16
berdirThis was a tricky one.
* $assert_session wasn't actually an assert session ;)
* waitForElementNotVisible() didn't actually exist, but assertNoElementAfterWait() has been added recently.
* Drupal core disabled animations in tests by default, that confused me for a while, have to set a flag to allow them in this specific test.
Also sneaking in a php 7.2 warning fix about count() in a kernel test.
Comment #18
berdirMuch green, such wow.
In you go.
Comment #19
martin107 commentedThank you, thank you