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.

Sign up for GitHub

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

Jump to bottom

Refactor engine client test_create_job test names for better clarity and consistency #6885

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Refactor engine client test_create_job test names for better clarity and consistency #6885

wants to merge 2 commits into from
+324 −264

Conversation

ayshiff
Copy link

@ayshiff ayshiff commented Dec 27, 2024 •
edited
Loading

Split initial test_create_job test into:

  • Four successful tests:
    • test_create_job_with_all_parameters
    • test_create_job_without_labels
    • test_create_job_without_description
    • test_create_job_without_specified_job_id
  • One failure test:
    • test_create_job_with_invalid_priority

Note: Splitting successful tests to stay consistent with existing tests:

  • test_create_job_with_invalid_processor_and_device_config_arguments_throws
  • test_create_job_with_run_name_and_device_config_name_succeeds

EDIT: Also refactored the test cases to use pytest fixtures for default_run_context and default_engine_client for consistency and to reduce redundancy.

Fixes #6761

@ayshiff ayshiff requested review from wcourtney, vtomole, verult and a team as code owners December 27, 2024 14:54
@CirqBot CirqBot added the size: M 50< lines changed <250 label Dec 27, 2024
@codecov Codecov
Copy link

codecov bot commented Jan 2, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.86%. Comparing base (06f12b9) to head (35d0cf5).
Report is 5 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #6885   +/-   ##
=======================================
  Coverage   97.86%   97.86%           
=======================================
  Files        1084     1084           
  Lines       94225    94321   +96     
=======================================
+ Hits        92215    92311   +96     
  Misses       2010     2010

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

@ayshiff ayshiff force-pushed the improve-test-create-job-names branch from 34fbebd to eddb152 Compare January 2, 2025 05:02
senecameeks
senecameeks reviewed Jan 6, 2025
View reviewed changes
Copy link
Collaborator

@senecameeks senecameeks left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! Consider assigning the original bug to yourself, thanks! #6761

cirq-google/cirq_google/engine/engine_client_test.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/engine/engine_client_test.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/engine/engine_client_test.py Outdated Show resolved Hide resolved
@ayshiff
Refactor test names for better clarity and consistency
2aca27c 
Split initial test_create_job into:
- Four successful tests:
  - test_create_job_with_all_parameters
  - test_create_job_without_labels
  - test_create_job_without_description
  - test_create_job_without_specified_job_id
- One failure test:
  - test_create_job_with_invalid_priority

Note: Splitting successful tests to stay consistent with existing tests:
  - test_create_job_with_invalid_processor_and_device_config_arguments_throws
  - test_create_job_with_run_name_and_device_config_name_succeeds
@ayshiff ayshiff force-pushed the improve-test-create-job-names branch from 35d0cf5 to 2aca27c Compare January 7, 2025 17:15
@ayshiff
Copy link
Author

ayshiff commented Jan 7, 2025

Thanks for contributing! Consider assigning the original bug to yourself, thanks! #6761

Thanks for the review! Unfortunately, I don't think I have permission to assign the original bug to myself.

@ayshiff
Copy link
Author

ayshiff commented Jan 8, 2025

Following @senecameeks's comment -> Refactored the test cases to use pytest fixtures for default_run_context and default_engine_client for consistency and to reduce redundancy. All instances of run_context = any_pb2.Any() and client = EngineClient() have been replaced with the respective fixtures.

@ayshiff
Refactor tests to use pytest fixtures for default_run_context and `…
b0723b0 
…default_engine_client`

- Updated all test cases to use these fixtures directly for consistency and clarity.
@ayshiff ayshiff force-pushed the improve-test-create-job-names branch from 778e886 to b0723b0 Compare January 8, 2025 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@senecameeks senecameeks senecameeks left review comments

@wcourtney wcourtney Awaiting requested review from wcourtney wcourtney is a code owner

@vtomole vtomole Awaiting requested review from vtomole vtomole is a code owner

@verult verult Awaiting requested review from verult verult is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
size: M 50< lines changed <250
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clean up test_create_job in cirq-google/cirq_google/engine/engine_client_test.py
3 participants
@ayshiff @senecameeks @CirqBot