Fixing waitForDisconnect to (optionally) wait for disconnects#1232
Fixing waitForDisconnect to (optionally) wait for disconnects#1232mattklein123 merged 5 commits intoenvoyproxy:masterfrom
Conversation
|
I think @lizan opened this patch already a while ago and we decided not to merge it because at the time it was indicative of a test that was not written correctly. In this case, it seems like there is a real reason to do it this way? I'm fine going with this fix if it makes writing tests easier, but perhaps can you add a comment to waitForDisconnect() saying why we do the while loop and what type of testing that is useful for? cc @htuch |
…hem to ignore spurious data
|
Sounds like there is some desire for that drain-by-default behavior. New patch - stick with the old default behavior, allow an easy override and comment heavily what's going on in both cases :-) |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Awesome looks great, thanks. Small typo.
test/integration/fake_upstream.cc
Outdated
| while (!disconnected_) { | ||
| connection_event_.wait(lock); | ||
| // The default behavior of waitForDisconnect is to assume the test cleanly | ||
| // calls waitForData, waitoforNewStream, etc. to handle all events on the |
There was a problem hiding this comment.
s/waitoforNewStream/waitForNewStream
alyssawilk
changed the title
**Description** This updates to the latest openinference semantic conventions for CreateEmbeddings requests, and re-records all spans. This also removes the ModelO3Mini as OpenAI themselves recently removed it. **Related Issues/PRs (if applicable)** #1085 Signed-off-by: Adrian Cole <adrian@tetrate.io>
3 participants
Prior integration tests drained all data before calling waitForDisconnect(), so were not affected by the condition variable being triggered by any changes to the connection, not just disconnects.
An alternate fix is to change the first 3 tcp proxy integration tests to do what the 4th does and actively drain fake_rest_connection before calling waitForDisconnect(), so waitForDisconnect's wait() can't be triggered by onData. This would result in arguably cleaner tests, insofar as all data and events will always be accounted for, but leaves us with a confusingly named function and a requirement to do extra work even when it's not relevant to the system under test. That's a super easy change as well so let me know if you'd prefer I go that route and I'll comment up waitForDisconnect as best I can :-)