-
Notifications
You must be signed in to change notification settings - Fork 321
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
Fix flaky TestCafe OV polling test in DeviceChallengePollView_spec
#3654
base: 7.21
Are you sure you want to change the base?
Conversation
2bba797
to
268edc4
Compare
0d087d5
to
054cb76
Compare
@@ -14,7 +14,13 @@ const request = (opts) => { | |||
method: 'GET', | |||
contentType: 'application/json', | |||
}, opts); | |||
return $.ajax(ajaxOptions); | |||
return new Promise((resolve, reject) => { |
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.
Explain why/how this helps
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.
@shuowu-okta did you have a specific behavior-change related concern w.r.t. Promisifying 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.
In current code Promises are mixed with jQuery's Deferred (line 159 let probeChain = Promise.resolve();
uses Promise, $.ajax
returns Deferred and done/fail
APIs are used)
Tests expect that requests are performed in chain: probe port 2000 -> challenge 2000 (if probe OK) -> probe 6511 -> challenge 6511 -> probe 6512 -> challenge 6512 -> probe 6513 -> challenge 6513
With current code it's not always true, eg. probe 6511
can run in parallel with challenge 2000
So eg. this check
await t.expect(loopbackChallengeErrorLogger.contains(record => record.request.url.match(/6512|6513/))).eql(false); |
can fail
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.
Changing the base xhr approach may cause unexpected issue for legacy browsers. As the goal is to fix flaky bacon test, I propose to find another way that fix against test directly, for example, migrate test from e2e to unit level (testcafe -> jest).
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.
See latest commit that uses $.Deferred
instead of Promise
and then+catch
API instead of done+fail
for proper chaining.
The recommended API method to use chaining is then
: https://api.jquery.com/deferred.then/
}; | ||
|
||
let probeChain = Promise.resolve(); | ||
let probeChain = $.Deferred().resolve(); |
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.
will this deferred cause any degradation of UX like slower authentication speed?
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.
With this change the behaviour will be same as in Gen3 - probe/challenge calls will be running not in parallel, but in chain (probing/challenging for port 2 should wait for probing/challenging of port 1 to complete).
This can be slower.
But such behaviour in expected judging from code and tests.
If it's desired to have parallel requests, then tests should be rewritten to reflect this.
For example, this line should be removed:
await t.expect(loopbackChallengeErrorLogger.contains(record => record.request.url.match(/6512|6513/))).eql(false); |
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.
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.
After talking with the team, we would like to fix it, to make V2 behave similar like V3, there shouldn't be a performance concern.
We can be more cautious by using a new flag from server, but this might be an overkill
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 typically isolate higher-risk changes in a patch bump on the tip so it can be easily rolled back. I think this can target 7.22.1
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.
the $.Deferred().resolve()
is the jquery's promise, not sure about the very low level, but jquery promise has the .done
method (similar as .then
), while es6 promise doesn't.
since all the probing will use jquery $.ajax method, so I guess using $.Deferred().resolve()
and return the rejected promise in onFailure
will make it serial, but I will leave it to @denysoblohin-okta to confirm
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.
How does using $.Deferred().resolve() differ from Promise.resolve()
First one returns $.Deferred
object instead of Promise
. This will change probeChain
from a chain of Promises to a chain of Deferred. Deferred
is a "thenable" object, but not a real promise.
how does it cause the probe chain to become serial?
In current code native Promises are mixed with jQuery's Deferred:
let probeChain = Promise.resolve(); |
return $.ajax(ajaxOptions); |
(
$.ajax
returns $.Deferred
)
probeChain
uses native Promises with then/catch
API. But doProbing()
returns $.Deferred
and uses done/fail
API.
This is a problem. To achieve proper chaining we need to change done/fail
API to then/catch
.
Actually we can leave Promise.resolve()
instead of $.Deferred().resolve()
, then we still have mixing of Promise and Deferred, but they can play well together with then/catch
API.
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.
Rebased PR onto 7.21
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.
just curious, this will target 7.21.2
in the 2024.09.0?
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.
Actually we can leave Promise.resolve() instead of $.Deferred().resolve()
Yeah, I think now that the Promise
API is standardized and canonically polyfilled we should try to use Promise
when we can since it's better understood and well-documented (MDN).
723f794
to
ffa12d6
Compare
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.
LGTM, if we want to be very cautious, we can put this fix behind a FF or KS, but this maybe an overkill
ffa12d6
to
b197e89
Compare
1e31c67
to
a5b414c
Compare
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.
LGTM!
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.
Hold for 7.22.1 release
Description:
The fix consists of 2 parts:
test.skip.requestHooks(xxx)
interferes with mocks in other tests - other tests in spec can use mocks from skipped test$.ajax
in BaseOktaVerifyChallengeView - migrated from jQuery's Promise-likedone/fail
to nativethen/catch
. With old approach,probe
andchallenge
requests are not chained correctly (et. probing port 6512 can be started in parallel with challenging port 6511). With native Promise it's always a chainprobe 1 -> challenge 1 -> probe 2 -> challenge 2 -> ...
Notes:
testcafe
taskPR Checklist
Issue:
Reviewers:
Screenshot/Video:
Downstream Monolith Build: