Don't use expect within Promises (#5466) #5473

whimboo · 2020-03-04T12:34:39Z

This PR will partly fix issue #5466.

If a call to expect fails within a Promise it will not
be resolved, and causing the test to crash.

The patch aligns the code similar to what is used by all
the other tests.

whimboo · 2020-03-04T13:00:02Z

@mathiasbynens, in regards of the Travis and AppVeyor failure for navitation.spec.js shouldn't await page.goto() wait until all the requests have been processed?

whimboo · 2020-03-25T20:44:12Z

test/navigation.spec.js

      let error = null;
      await page.goto(httpsServer.EMPTY_PAGE).catch(e => error = e);
      if (CHROME)
        expect(error.message).toContain('net::ERR_CERT_AUTHORITY_INVALID');
      else
        expect(error.message).toContain('SSL_ERROR_UNKNOWN');
+      expect(requests.length).toBe(3);
+      for (const request of requests)
+        expect(request).toBeTruthy();


@mathiasbynens this change actually caused tests to fail for Chrome:
https://travis-ci.com/github/puppeteer/puppeteer/jobs/293846770#L543

Any suggestion what has to be done to get this fixed? Maybe waiting until the length is 3?

jackfranklin · 2020-04-17T12:32:33Z

Hey @whimboo I'm afraid we've got some conflicts now we've done some updates to the test runner (we now use Mocha rather than a custom one). Would you be up for rebasing and landing this work? I'm happy to work with you to figure out that navigation.spec.js test if it's still causing issues after a rebase.

whimboo · 2020-05-12T14:36:13Z

Hey @jackfranklin. Sorry for not replying earlier but I missed the Github notification. I will have a look in updating this PR (in case the changes are still needed). Maybe I will have something later today.

jackfranklin · 2020-05-12T14:39:45Z

@whimboo no probs! Ping me if I can help :)

whimboo · 2020-05-12T15:12:35Z

I hope the recently pushed changes will work. I'm currently not able to test locally because of the following failure message:

Error: Cannot find module './lib/helper'
Require stack:
- /Users/henrik/code/puppeteer/index.js
- /Users/henrik/code/puppeteer/test/mocha-utils.js
- /Users/henrik/code/puppeteer/node_modules/mocha/lib/mocha.js
- /Users/henrik/code/puppeteer/node_modules/mocha/lib/cli/one-and-dones.js
- /Users/henrik/code/puppeteer/node_modules/mocha/lib/cli/options.js
- /Users/henrik/code/puppeteer/node_modules/mocha/bin/mocha

And there is not such a lib folder.

jackfranklin · 2020-05-12T15:16:39Z

@whimboo can you run node install.js or npm run tsc?

whimboo · 2020-05-12T15:23:05Z

Oh node install.js worked. Can we get this step added to https://github.com/puppeteer/puppeteer/blob/master/test/README.md?

jackfranklin · 2020-05-12T15:43:17Z

@whimboo #5852 should clarify the testing part

whimboo · 2020-05-12T16:30:02Z

@jackfranklin the problem with the navigation test failure still exists:
https://travis-ci.com/github/puppeteer/puppeteer/jobs/332575437#L289-L297

whimboo · 2020-05-12T16:37:52Z

I assume this is a bug in the test and that we should not expect requestfinished to be fired, but only requestfailed. Modifying the test I see the following events being emitted:

*** request
*** requestfailed

whimboo · 2020-05-12T19:58:23Z

@jackfranklin if those checks are fine, all the tests passing now.

jackfranklin · 2020-05-13T08:25:16Z

test/navigation.spec.js

+      const requests = [];
+      page.on('request', (request) => requests.push('request'));
+      page.on('requestfinished', (request) => requests.push('requestfinished'));
+      page.on('requestfailed', (request) => requests.push('requestfailed'));


@whimboo to check my understanding; do you think this test used to pass because we've never get a requestfinished event and therefore that expectation would never run?

@mathiasbynens do you have any thoughts on if we should be getting the requestfinished event? I think it makes sense that we don't, given that this test checks that requests fail and therefore I wouldn't expect the requestfinished event necessarily. I think this was a mistake in the test that made it in and happened to not cause any issues.

Yes, exactly. I think we either get requestfinished or requestfailed. But agreed that this is something @mathiasbynens should reply on.

whimboo · 2020-05-13T11:02:30Z

@jackfranklin also I assume half of the changes for this PR are super-seeded by your PR #5859?

jackfranklin · 2020-05-13T13:48:59Z

@whimboo ah yes looks like it might be - sorry to cause more work for you :( Let's get #5859 and then rebase this PR. Apologies for the back and forth.

whimboo · 2020-05-14T05:40:04Z

No problem at all. Are the changes to the navigation tests ok, or would they have to be updated similar to your dialog changes?

jackfranklin · 2020-05-21T14:59:01Z

@whimboo apologies, this slipped off my radar. I just tried to rebase on GitHub but I've made a mistake and now the tests are failing. If you could tidy up the failing test on your fork and push then this will be good to merge. Thanks!

whimboo · 2020-05-25T06:00:46Z

test/dialog.spec.js

@@ -54,6 +57,9 @@ describe('Page.Events.Dialog', function () {
    expect(dialog.message()).toBe('question?');

    expect(result).toBe('answer!');
+    expect(type).toBe('prompt');
+    expect(defaultValue).toBe('yes.');
+    expect(message).toBe('question?');


@jackfranklin do we still need those changes or are all of these obsolete now? Also what about the other test above. I assume it needs a similar call to sinon.stub().callsFake()?

I just pushed an update with that. So let me know if that is fine with you.

If a call to expect fails within a Promise it will not be resolved, and causing the test to crash. The patch aligns the code similar to what is used by all the other tests.

whimboo · 2020-05-25T08:08:56Z

Tests are failing because of Error: src/protocol.d.ts is out of date.. I pushed another update.

jackfranklin · 2020-05-26T08:22:24Z

@whimboo thanks! Sorry again for the back and forth on this one.

googlebot added the cla: yes label Mar 4, 2020

whimboo force-pushed the expect_in_callback branch 2 times, most recently from 28da0f6 to 2a17630 Compare March 4, 2020 12:41

AlvaroMolinaBarquera mentioned this pull request Mar 4, 2020

FileChooser.accept not selecting the files #5474

Closed

mathiasbynens approved these changes Mar 10, 2020

View reviewed changes

whimboo commented Mar 25, 2020

View reviewed changes

mathiasbynens added the firefox label Apr 17, 2020

whimboo force-pushed the expect_in_callback branch from 2a17630 to b814774 Compare May 12, 2020 15:09

whimboo force-pushed the expect_in_callback branch from b814774 to 3a2509c Compare May 12, 2020 15:29

whimboo force-pushed the expect_in_callback branch from 3a2509c to c6c1435 Compare May 12, 2020 16:23

whimboo force-pushed the expect_in_callback branch 2 times, most recently from 0e917d5 to 13b0545 Compare May 12, 2020 19:18

jackfranklin reviewed May 13, 2020

View reviewed changes

whimboo mentioned this pull request May 13, 2020

chore: Fix async dialog specs when they fail #5859

Merged

whimboo commented May 25, 2020

View reviewed changes

whimboo force-pushed the expect_in_callback branch from 7b196cf to 000b3b8 Compare May 25, 2020 07:02

whimboo added 2 commits May 25, 2020 10:07

Don't use expect within Promises (puppeteer#5466)

6e2d000

If a call to expect fails within a Promise it will not be resolved, and causing the test to crash. The patch aligns the code similar to what is used by all the other tests.

Update protocol definition.

e0857d7

whimboo force-pushed the expect_in_callback branch from 000b3b8 to e0857d7 Compare May 25, 2020 08:08

jackfranklin merged commit 6cfe142 into puppeteer:master May 26, 2020

whimboo deleted the expect_in_callback branch May 26, 2020 08:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use expect within Promises (#5466) #5473

Don't use expect within Promises (#5466) #5473

whimboo commented Mar 4, 2020 •

edited

whimboo commented Mar 4, 2020

whimboo Mar 25, 2020

jackfranklin commented Apr 17, 2020

whimboo commented May 12, 2020

jackfranklin commented May 12, 2020

whimboo commented May 12, 2020

jackfranklin commented May 12, 2020 •

edited

whimboo commented May 12, 2020

jackfranklin commented May 12, 2020

whimboo commented May 12, 2020

whimboo commented May 12, 2020

whimboo commented May 12, 2020

jackfranklin May 13, 2020

whimboo May 13, 2020

whimboo commented May 13, 2020

jackfranklin commented May 13, 2020

whimboo commented May 14, 2020

jackfranklin commented May 21, 2020

whimboo May 25, 2020

whimboo May 25, 2020

whimboo commented May 25, 2020

jackfranklin commented May 26, 2020

Don't use expect within Promises (#5466) #5473

Don't use expect within Promises (#5466) #5473

Conversation

whimboo commented Mar 4, 2020 • edited

whimboo commented Mar 4, 2020

whimboo Mar 25, 2020

Choose a reason for hiding this comment

jackfranklin commented Apr 17, 2020

whimboo commented May 12, 2020

jackfranklin commented May 12, 2020

whimboo commented May 12, 2020

jackfranklin commented May 12, 2020 • edited

whimboo commented May 12, 2020

jackfranklin commented May 12, 2020

whimboo commented May 12, 2020

whimboo commented May 12, 2020

whimboo commented May 12, 2020

jackfranklin May 13, 2020

Choose a reason for hiding this comment

whimboo May 13, 2020

Choose a reason for hiding this comment

whimboo commented May 13, 2020

jackfranklin commented May 13, 2020

whimboo commented May 14, 2020

jackfranklin commented May 21, 2020

whimboo May 25, 2020

Choose a reason for hiding this comment

whimboo May 25, 2020

Choose a reason for hiding this comment

whimboo commented May 25, 2020

jackfranklin commented May 26, 2020

whimboo commented Mar 4, 2020 •

edited

jackfranklin commented May 12, 2020 •

edited