Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Really fix selenium #3647

Merged
merged 2 commits into from
Oct 18, 2017
Merged

Really fix selenium #3647

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ad2037
Revert "Fix #3616, retry saveButton.click() in tests (#3645)"
ianb Oct 17, 2017
1c773ba
Fix #3616, avoid intermittent failures in Selenium test
ianb Oct 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 6 additions & 36 deletions test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,36 +114,6 @@ function promiseFinally(promise, finallyCallback) {
});
}

/** Retry some promise-generating code several times.
promiseGenerator: Like () => {return somethingThatMakesAPromise();}
times: number of times to try
wait: milliseconds to wait between runs (default 1000ms)
catchExceptions: optional, a function that indicates if this exception should
be caught. Like (exc) => {return exc instanceof Error;}
*/
function retryPromise(promiseGenerator, times, wait, catchExceptions) {
if (wait === undefined) {
wait = 1000;
}
if (!catchExceptions) {
catchExceptions = exc => true;
}
let tried = 0;
function start() {
let promise = promiseGenerator();
tried++;
return promise.catch((exc) => {
if (tried <= times && catchExceptions(exc, tried)) {
return setTimeoutPromise(wait).then(() => {
return start();
});
}
throw exc;
});
}
return start();
}

function setTimeoutPromise(time) {
return new Promise((resolve, reject) => {
setTimeout(resolve, time);
Expand Down Expand Up @@ -281,6 +251,11 @@ describe("Test Screenshots", function() {
return skipOnboarding(driver);
}).then(() => {
return focusIframe(driver, PRESELECTION_IFRAME_ID);
}).then(() => {
// This avoids a problem where the UI has been instantiated, and handlers
// added, but not everything is fully setup yet; by waiting we give it
// time to set everything up
return setTimeoutPromise(500);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're here again, the right way to fix this, according to webdriver best practices, is to use an implicit or explicit wait, rather than bake a long delay into the tests. @davehunt any suggestions on how to ensure webdriver waits for iframed UI to be fully ready?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to fix this we'd have to do something a bit fancier. The elements appear (it does wait), but if you click them right away there are problems.

This display code is the issue:

screenshots/addon/webextension/selector/ui.js

Lines 414 to 418 in 2abb4de

display(installHandlerOnDocument, standardOverlayCallbacks) {
return iframeSelection.display(installHandlerOnDocument)
.then(() => iframePreSelection.display(installHandlerOnDocument, standardOverlayCallbacks))
.then(() => iframePreview.display(installHandlerOnDocument, standardOverlayCallbacks));
},

All of iframeSelection.display, iframePreSelection.display and iframePreview.display have to complete before you click on elements. They all complete in a reasonable time, but not reasonable for a computer that's polling for the interface.

To fix this properly we'd have to keep that promise, and use it to guard all the actual functions in installHandlerOnDocument and standardOverlayCallbacks. It felt a bit too complicated, though I suppose it's only 15ish lines of code when I think about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Followup in #3653

}).then(() => {
return driver.wait(
until.elementLocated(By.css(".visible"))
Expand All @@ -300,12 +275,7 @@ describe("Test Screenshots", function() {
);
}).then((saveButton) => {
return expectCreatedShot(driver, () => {
return retryPromise(() => {
return saveButton.click();
}, 3, 1000, (exc, times) => {
console.log("Retry", times, "of saveButton.click() because of error:", exc);
return true;
});
saveButton.click();
});
}).then((shotUrl) => {
assert(shotUrl.startsWith(backend), `Got url ${shotUrl} that doesn't start with ${backend}`);
Expand Down