Skip to content
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

Changed FakeQuantumRunStream to support arbitrary response and exception timing #6253

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 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
33 checks passed
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.

None yet

3 participants