[Flaky tests] Metric's server collector's integration tests #127139
Conversation
afharo
added
release_note:skip
|
|
||
| router.get({ path: '/', validate: false }, async (ctx, req, res) => { | ||
| return res.ok({ body: '' }); | ||
| }); | ||
| router.get({ path: '/disconnect', validate: false }, async (ctx, req, res) => { | ||
| hitSubject.next(hitSubject.value + 1); |
There was a problem hiding this comment.
I went for a plain Subject vs. BehaviourSubject(counter) just in case it was affected by concurrent read/writes:
- The 2
/disconnectrequests occur at a similar time - This line is executed when
hitSubject.valuestill holds0for both requests.
IMO, for the purpose of the test, it's enough relying on the .pipe(take(2)) approach. What do you think?
There was a problem hiding this comment.
hitSubject.next(hitSubject.value + 1); is synchronous, so 'atomical' for the event loop. I don't think there's any risk for concurrency problems. But using a plain subject and counting the emissions works fine too.
There was a problem hiding this comment.
Thanks for explaining! I was mostly concerned if calling .next would synchronously update .value to consider it atomic. Since it's a RxJS internal implementation, I was not sure. Glad to know that's the case!
There was a problem hiding this comment.
I was mostly concerned if calling .next would synchronously update .value to consider it atomic
AFAIK it does (it even call the current subscribers synchronously), but I agree that we should probably consider this as an implementation detail of RXJS
| @@ -125,7 +121,8 @@ describe.skip('ServerMetricsCollector', () => { | |||
| ); | |||
|
|
|||
| discoReq2.abort(); | |||
| await delay(requestWaitDelay); | |||
| // Wait for the aborted$ event | |||
| await disconnectAborted$.pipe(take(1)).toPromise(); | |||
There was a problem hiding this comment.
Similar to my previous comment: we could use a BehaviourSubject(counter). It could seem more explicit since we would check the counter === 2 vs. this approach that looks for increments. However, it feels concurrently safer this approach.
What do you think?
|
@elasticmachine merge upstream |
|
|
||
| router.get({ path: '/', validate: false }, async (ctx, req, res) => { | ||
| return res.ok({ body: '' }); | ||
| }); | ||
| router.get({ path: '/disconnect', validate: false }, async (ctx, req, res) => { | ||
| hitSubject.next(hitSubject.value + 1); |
There was a problem hiding this comment.
hitSubject.next(hitSubject.value + 1); is synchronous, so 'atomical' for the event loop. I don't think there's any risk for concurrency problems. But using a plain subject and counting the emissions works fine too.
| const discoReq1 = sendGet('/disconnect').end(); | ||
| const discoReq2 = sendGet('/disconnect').end(); | ||
|
|
||
| // Wait for 2 requests to /disconnect | ||
| await disconnectRequested$.pipe(take(2)).toPromise(); |
There was a problem hiding this comment.
NIT: we're subscribing to the observable after having sent the requests. I think the risk of missing an emission is kinda non-existent, but just to be safe, I'd either use a replay effect on disconnectRequested$, or:
const disconnectRequested = disconnectRequested$.pipe(take(2)).toPromise();
const discoReq1 = sendGet('/disconnect').end();
const discoReq2 = sendGet('/disconnect').end();
await disconnectRequested;There was a problem hiding this comment.
Good catch! I'll make sure to subscribe before triggering any event.
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 6bcce44)
💔 Some backports could not be created
Manual backportTo create the backport manually run: Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
Summary
Resolves #59234.
It uses the
aborted$event to identify when the requests' abort actually happen instead of relying on delays that proved to be flaky.NOTE: There's the risk that the
aborted$emitter wouldn't work as expected #125240. But it should be fixed at the emitter's level. IMO, these tests should consider the emitter just works.Checklist
For maintainers
Backporting strategy
I think it's safe to backport these to the active branches
8.1and7.17because it's only reenabling the tests, and it will provide our releases with more confidence in test coverage. Should we also backport it to8.0or is it OK to leave that gap?