Page MenuHomePhabricator

Investigate the failures in 'can add a statement using the keyboard' browser test'
Closed, ResolvedPublic

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 9 2019, 2:15 PM
alaa_wmde added a comment.EditedAug 9 2019, 2:34 PM

For this one, we got back to the original problems we faced with it and why we had to introduce browser.pause in the first place. It appears that sometimes the 1 second we pause might not always be enough, as locally watching the test we could see that the element approached almost a second to appear (at least it didn't feel that instant) so we imagined that it could be in some runs of the test that it takes more than a second which makes it fail, due to invalid form as the keys entered using browser.keys either do not end up in any input (making qualifier input invalid as it is empty) or end up in qualifier property input field (making it invalid too for a wrong property id/label there).

The *unexpected behavior* on WDIO side

*snippet for reference*

               let statement = ItemPage.statements[ 0 ];
		ItemPage.getNthReferencePropertyInput( statement, 0 ).waitForVisible();
		browser.keys( propertyId );
		// value input automatically focused
		ItemPage.getNthReferenceValueInput( statement, 0 ).waitForExist();
		browser.pause( 1000 );
		// value input automatically focused
		browser.keys( 'reference 1-1' );
		browser.pause( 1000 );
		browser.waitUntil( () => {
			return ItemPage.isSaveButtonEnabled();
		} );

So we remembered (re-discovered effectively) that we had to use browser.pause because calling ItemPage.getNthReferenceValueInput( statement, 0 ).waitForExist(); actually returns true immediately regardless whether that element existed or not.

The element returned by calling ItemPage.getNthReferenceValueInput( statement, 0 ) is constructed this way:

// source

let qualifier = statement.$$( this.constructor.ITEM_WIDGET_SELECTORES.QUALIFIERS )[ qualifierIndex ];
return qualifier.$( this.constructor.ITEM_WIDGET_SELECTORES.VALUE_INPUT );

So the returned element is constructed as a "child" through querying the selector .valueview-input on the parent qualifier element.

Then we noticed that there was another element on the page that had the selector .valueview-input (value input for property itself). So we suspected that calling waitForVisible on the element is actually returning true because, unexpectedly, it might be checking for elements in the page with selector .valueview-input without constraining its search to elements within the parent it was constructed from.

To test that hypothesis, we instead used a full selector in our test and dropped all browser.pause (dropping the pauses would've made the test fail before), and voila! it worked as expected now.

*snippet of the test we did for reference*

let statement = ItemPage.statements[ 0 ];
ItemPage.getNthQualifierPropertyInput( statement, 0 ).waitForVisible();
browser.keys( propertyId );
// value input automatically focused
const selector = '.wikibase-statementview-qualifiers .listview-item:first-child .valueview-input'
const node = $(selector);

browser.keys( 'qualifier 1' );

browser.waitUntil( () => {
    return ItemPage.isSaveButtonEnabled();
} );

So next thing to verify is whether that behavior was discovered as a bug and solved in latest stable of wdio. In that case we probably only need to upgrade.

If that's not the case, or we cannot upgrade for whatever reasons, then we will have to change Item page object to actually use full selectors instead, as well as not reuse elements within the test by creating element instances for every step (esp. when we expect things to change in DOM) as a general principal.

Change 529776 had a related patch set uploaded (by Noa wmde; owner: Noa wmde):
[mediawiki/extensions/Wikibase@master] Fix browser test to use a more reliable and exact selector

@alaa_wmde I made a patch which could be abandoned in case we continue with the option of upgrading wdio.
The item pagepbject could also do with some cleaning up (mostly selectors which we ended up not using)

Change 529776 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Fix browser test to use a more reliable and exact selector

I think it's best to leave it under test verification for a bit, to see if we get any ricochets from this being back on CI

noarave closed this task as Resolved.Aug 23 2019, 10:54 AM
noarave claimed this task.

No CI issues related to this test reported this week, resolving.

Restricted Application added a project: User-Noarave. · View Herald TranscriptAug 23 2019, 10:54 AM
DannyS712 added a subscriber: DannyS712.

[batch] remove patch for review tag from resolved tasks

Restricted Application added a project: Wikidata. · View Herald TranscriptNov 18 2019, 11:35 AM