Skip to content

[TEST] Add some tests for remote extractors#95

Merged
MatthieuMoreau0 merged 6 commits intomainfrom
mm/mock-sdk
Mar 31, 2025
Merged

[TEST] Add some tests for remote extractors#95
MatthieuMoreau0 merged 6 commits intomainfrom
mm/mock-sdk

Conversation

@MatthieuMoreau0
Copy link
Collaborator

Adding some tests for the remote extractors as per the dedicated issue

This is a first draft, I'm not super happy with the current implementation which imo relies too heavily on mocks and hides a lot of details which could break scripts running qek on remote backends. However this is better than no tests at all and gives a first view on what tests for remote backends could look like.

I think the MockSDK class should be eventually moved to the pasqal-cloud repository so that it can be 1) maintained alongside the library whenever we make changes; 2) reused easily for all projects that rely on pasqal cloud and want to do some tests without calling the cloud services.

Anyways curious to see what you guys think.

@Yoric Yoric requested review from RolandMacDoland and Yoric March 6, 2025 09:52
Copy link
Contributor

@Yoric Yoric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

We'll want to also add tests for backends, but one your MockSDK is in the tree, we can do that ourselves.

Copy link
Contributor

@RolandMacDoland RolandMacDoland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One point though that I've mentioned above, why not providing some base MockSDK from pasqal_cloud that we can customize for the needs of testing APIs that use it ? @Yoric @MatthieuMoreau0

@MatthieuMoreau0
Copy link
Collaborator Author

MatthieuMoreau0 commented Mar 20, 2025

@Yoric @RolandMacDoland I have made some changes to this MR. The scope has gone a bit beyond pure tests. Indeed as I was writing tests for this I realise that qek doesn't care so much about batches but only the job inside each batch. I slightly reworked the remote extractors class to account for that and store jobs instead of batches. This makes the code slightly simpler, imo. Let me know if that's ok for you.

I have also updated the test structure. Now the the mock server simulates the execution of the jobs. Each time the get_job method is called, the progress of the job goes one step forward PENDING => RUNNING => DONE. This mimicks the async nature of the remote execution and makes sure qek properly waits for jobs to be done and for results to be available. Looking for feedback on this

Copy link
Contributor

@Yoric Yoric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty nice, thanks!

@Yoric
Copy link
Contributor

Yoric commented Mar 21, 2025

@MatthieuMoreau0 So far, this looks good, plus the tests seem to pass, which helps :) What are the next steps?

@MatthieuMoreau0
Copy link
Collaborator Author

MatthieuMoreau0 commented Mar 21, 2025

@Yoric I'll update my MR with your feedback. Once this is done I think this can be merged; this will provide basic coverage for remote backends nominal behaviour.

Next steps would be:

  • adding tests for backends as you mentioned in a comment above
  • adding tests for error cases (when a job fails for some reason)
  • adding tests for resuming job execution. Looks like a list of job IDs can be passed to the extractor, we should ensure this behaves as expected with the mockSDK
  • moving over the mock SDK to the pasqal cloud package

@MatthieuMoreau0 MatthieuMoreau0 marked this pull request as ready for review March 24, 2025 18:40
@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2025

Codecov Report

❌ Patch coverage is 2.36220% with 124 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.91%. Comparing base (b31b9db) to head (53850cb).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
qek/data/extractors.py 5.66% 50 Missing ⚠️
tests/mock_cloud_sdk.py 0.00% 45 Missing ⚠️
tests/test_extractors.py 0.00% 28 Missing ⚠️
qek/target/backends.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #95      +/-   ##
==========================================
- Coverage   42.49%   39.91%   -2.59%     
==========================================
  Files          16       17       +1     
  Lines        1106     1175      +69     
  Branches      132      136       +4     
==========================================
- Hits          470      469       -1     
- Misses        607      677      +70     
  Partials       29       29              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@MatthieuMoreau0 MatthieuMoreau0 merged commit c819a79 into main Mar 31, 2025
23 checks passed
@MatthieuMoreau0 MatthieuMoreau0 deleted the mm/mock-sdk branch March 31, 2025 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants