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

test: Mock only service call in AHS device initialization #229

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

yitchen-tim
Copy link
Contributor

@yitchen-tim yitchen-tim commented Feb 2, 2024

Description of changes:
Add noise model to mock aws device in unit test. This is an update for introducing BDK PR amazon-braket/amazon-braket-sdk-python#850

Testing done:
tox

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

Tests

  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have checked that my tests are not configured for a specific region or account (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@yitchen-tim yitchen-tim requested a review from a team as a code owner February 2, 2024 01:33
Copy link

codecov bot commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2af722e) 100.00% compared to head (db97c89) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #229   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines         1185      1185           
  Branches       286       286           
=========================================
  Hits          1185      1185           

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

@speller26
Copy link
Contributor

Why not include this in #850? The change doesn't make sense on its own.

@yitchen-tim
Copy link
Contributor Author

yitchen-tim commented Feb 2, 2024

@speller26 I didn't see a way for a PR to make changes in two different repository? Is there a better way than having two separate PRs?

@speller26
Copy link
Contributor

Oops, thought this one was also in the SDK; in that case we'll just hold off until that PR is finalized. In the meantime, the prefix of the PR should be test: rather than fix:

@speller26
Copy link
Contributor

The fact that we need to add the field here before we merge the SDK change indicates a problem with how our mocks are set up; we shouldn't have to add in a yet-to-be-introduced property here

@yitchen-tim
Copy link
Contributor Author

yes, it's related to this mock. It should mock the individual attributes, instead of filling in what's necessary to a Mock.

@speller26
Copy link
Contributor

Instead of adding a _noise_model attribute to the mock device for the test case, try replacing

m.setattr(AwsDevice, "__init__", lambda self, *args, **kwargs: None)
m.setattr(AwsDevice, "aws_session", MockAwsSession)

with

m.setattr(AwsDevice, "_get_session_and_initialize", lambda self, *args, **kwargs: MockAwsSession) 

Instead of mocking out the entirety of __init__, and all the useful attributes that come with it, this will only mock out the service call, which should be much more robust.

@yitchen-tim yitchen-tim changed the title fix: consider noise model in test fix: mock only service call in aws device initialization Feb 7, 2024
@yitchen-tim
Copy link
Contributor Author

Great, this solution is working! I also tested locally that amazon-braket/amazon-braket-sdk-python#850 will pass with this fix.

@speller26 speller26 changed the title fix: mock only service call in aws device initialization test: Mock only service call in AHS device initialization Feb 7, 2024
@speller26 speller26 merged commit 863b492 into main Feb 7, 2024
15 checks passed
@speller26 speller26 deleted the yitchen/noise-model-tests branch February 7, 2024 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants