Skip to content

Conversation

verult
Copy link
Collaborator

@verult verult commented Aug 21, 2023

The previous version of FakeQuantumRunStream has the limitation that a response or exception is only fired when it receives a request. This makes it impossible to test these scenarios:

  • Raising an exception before receiving a request or after sending a response
  • Respond to a request which is not the most recently received one.

This version supports both cases:

  • Tests can call reply() to send a response or raise an exception at any moment.
  • Tests can set the response message_id to one from a different request.

With this functionality, it would be possible to write a test for sending requests after breaking a stream, which would have revealed a bug discovered through manual testing.

@maffoo @wcourtney

@verult verult requested review from maffoo and wcourtney August 21, 2023 20:59
@verult verult requested review from vtomole, cduck and a team as code owners August 21, 2023 20:59
@CirqBot CirqBot added the size: L 250< lines changed <1000 label Aug 21, 2023
@verult
Copy link
Collaborator Author

verult commented Aug 25, 2023

The latest commit addressed several issues:

  • Bypasses typecheck errors in faking QuantumEngineServiceAsyncClient
  • Fixed "task got bad yield" errors due to asyncio.Queue() being initialized in the duet thread.
  • Introduced the ability to wait for multiple requests to be sent in FakeQuantumRunStream.wait_for_requests(). Otherwise, test_submit_twice_and_break_stream_expect_result_responses might flake because only one of the two expected GetQuantumResultRequests may have been sent after the ServiceUnavailable exception.

@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (6fae409) 97.88% compared to head (2ef0730) 97.88%.

❗ Current head 2ef0730 differs from pull request most recent head db9552a. Consider uploading reports for the commit db9552a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6253      +/-   ##
==========================================
- Coverage   97.88%   97.88%   -0.01%     
==========================================
  Files        1104     1106       +2     
  Lines       95692    95778      +86     
==========================================
+ Hits        93667    93751      +84     
- Misses       2025     2027       +2     
Files Changed Coverage Δ
...q-google/cirq_google/engine/stream_manager_test.py 100.00% <100.00%> (ø)

... and 39 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@verult verult enabled auto-merge (squash) September 7, 2023 22:47
@verult verult merged commit deedb45 into quantumlib:master Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: L 250< lines changed <1000
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants