Workbench: cleanup the process shutdown routines #1364
Conversation
There was a problem hiding this comment.
Thanks as always @davemfish for iterating and making the Workbench more stable. Just had a minor comment.
| @@ -133,13 +133,15 @@ beforeEach(() => { | |||
|
|
|||
| afterEach(async () => { | |||
| try { | |||
| const pages = await BROWSER.pages(); | |||
| await Promise.all(pages.map(page => page.close())); | |||
| @@ -232,11 +235,13 @@ test('Check local userguide links', async () => { | |||
| const downloadModal = await page.waitForSelector('.modal-dialog'); | |||
| const downloadModalCancel = await downloadModal.waitForSelector( | |||
| 'aria/[name="Cancel"][role="button"]'); | |||
| await page.waitForTimeout(500); // waiting for click handler to be ready | |||
There was a problem hiding this comment.
Any reason this one is longer than the others? I don't think it's necessary if different click events could need different wait times but would it be convenient to have a constant variable defining these wait values?
There was a problem hiding this comment.
I don't know why it's different so I've made it the same, and I added a variable.
But it's likely each button is different, in terms of when it becomes functional. It seems dependent on whether the javascript containing the click handler's function has loaded yet, or whether the element is visible yet, or otherwise visually blocked somehow. All we know for sure is that the element is present in the DOM (that's what the waitForSelector accomplishes). But these other things remain unknown.
There was a race condition in the shutdown routine of the workbench. It would mainly come up during the Windows puppeteer test, where the python subprocess would not be given a chance to exit before electron & jest exited. Using
execSyncinstead ofexecto calltaskkillseems reliable.Fixed #1356 by removing the potential for an infinite loop while shutting down the python process. Polling the server to see if it was still live was actually papering over the real problem of the race condition mentioned above.
Fixes #1362 by making sure to close instances of pptr
pagesbefore trying to close thebrowser.Unrelated to that, tests were sometimes unreliable because the script was trying to click on buttons before their click handlers were ready. I added some pauses in the execution to accommodate that and things appear reliable now.
Checklist