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
Unskip amp-consent e2e test #27662
Unskip amp-consent e2e test #27662
Conversation
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.
Thanks @zhouyx for looking into this!
await new Promise((resolve) => { | ||
setTimeout(resolve, 1000); | ||
}); |
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 believe in test-amp-consent-server-side-expire-cache.js
we are waiting 3 seconds instead of 1, do you think 1 second will suffice 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.
Ha didn't know we have await sleep()
, I'll switch to that.
1 sec seemed to work fine. Any reason 3 seconds is chosen? We can always extend later.
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 think you'll have to import it though! It's called sleep-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.
oops... 😳
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.
iirc, @estherkim found 3 seconds to be the most reliable for that test
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.
ok, switched to 3secs
I can't reproduce the test failure locally. Tried the fix a few times on travis, seemed to work.
Confirmed with @estherkim
await expect(url).to.have.been.sent
only check the urls till the check is invoked.I added a timeout before verifying the outgoing request to minimize the flakiness. An ideal fix would be to have something like
await expect(url).to.have.eventually.been.sent
.Closes #27634