-
Notifications
You must be signed in to change notification settings - Fork 352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Wait for all handled requests to resolve via .flush()
#75
Conversation
packages/@pollyjs/core/src/polly.js
Outdated
const NOOP = () => {}; | ||
|
||
await Promise.all( | ||
this._requests.map(r => Promise.resolve(r.promise).then(NOOP, NOOP)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats the role of the noops here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the promise can be resolved or rejected, the NOOP just catches both to make sure the Promise.resolve doesnt receive an uncaught promise.
.flush()
.flush()
* | ||
* @returns {Promise} | ||
*/ | ||
export default class DeferredPromise extends Promise { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be part of core since it's only used there
tests/integration/adapter-tests.js
Outdated
|
||
// Wait for all requests to be made | ||
// (Puppeteer adapter takes some time to make the actual request) | ||
while (requests.length !== 3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this only noticed within the puppeteer adapter? Worry we could mask future bugs by doing this always in all the adapter tests. I wonder also if this could be mitigated by resolving the flush promise in the next frame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this only noticed within the puppeteer adapter?
Yes.
We use these tests in many places for both node and browser. I'm not really sure whats a better way to do this while making sure all adapters are tested for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder also if this could be mitigated by resolving the flush promise in the next frame.
What do you mean by this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use these tests in many places for both node and browser. I'm not really sure whats a better way to do this while making sure all adapters are tested for.
Skip the test in puppeteer. Then create a test with this while loop work around within puppeteer's test suite so that we don't mask future bugs on the other adapters.
I wonder also if this could be mitigated by resolving the flush promise in the next frame.
Lets chat, I'd like to know more about what is going on here. Specifically "takes some time". Do we know how much time? Does this while loop loop more than once?
this.fetchRecord(); | ||
this.fetchRecord(); | ||
|
||
// Since it takes time for Puppeteer to execute the request in the browser's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should document this edge case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, there are no currently known edge cases. This is a test nuance that only relates to this test scenario.
Resolves #71.