Merge base branch and align test structure for batch actions#112
Merge base branch and align test structure for batch actions#112andystaples merged 5 commits intoandystaples/add-batch-actionsfrom
Conversation
Co-authored-by: andystaples <77818326+andystaples@users.noreply.github.com>
Co-authored-by: andystaples <77818326+andystaples@users.noreply.github.com>
|
@andystaples Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
andystaples
left a comment
andystaples
left a comment
There was a problem hiding this comment.
@copilot please address
| def test_get_orchestration_state_by_max_instance_count(): | ||
| with DurableTaskSchedulerWorker(host_address=endpoint, secure_channel=True, | ||
| taskhub=taskhub_name, token_credential=None) as w: | ||
| w.add_orchestrator(empty_orchestrator) | ||
| w.start() | ||
|
|
||
| c = DurableTaskSchedulerClient(host_address=endpoint, secure_channel=True, | ||
| taskhub=taskhub_name, token_credential=None) | ||
|
|
||
| # Query with max_instance_count | ||
| orchestrations = c.get_orchestration_state_by(max_instance_count=2) | ||
|
|
||
| # Should return at most 2 instances | ||
| assert len(orchestrations) <= 2 |
There was a problem hiding this comment.
@copilot how does this test differentiate between get_orchestration_state_by returning <=2 rows because the test was run before three or more orchestrations were scheduled vs. because the backend respected the limit in the request?
There was a problem hiding this comment.
Fixed in 40d411f. The test now creates exactly 3 orchestrations before querying with max_instance_count=2 and asserts exactly 2 are returned, ensuring we're testing that the backend respects the limit.
| def empty_entity(ctx: entities.EntityContext, _): | ||
| if ctx.operation == "delete": | ||
| ctx.delete_state() |
There was a problem hiding this comment.
@copilot can you use class-based entities and leverage the built-in "delete" operation?
There was a problem hiding this comment.
Updated in 40d411f. Now using a class-based EmptyEntity(entities.DurableEntity) which inherits the built-in delete() operation, making the test cleaner and more idiomatic.
|
|
||
| # Verify clean result | ||
| assert result.empty_entities_removed >= 0 | ||
| assert result.orphaned_locks_released >= 0 |
There was a problem hiding this comment.
@copilot I don't think there will be any orphaned locks released here, since we're not locking any entities. We likely won't be able to reproduce orphaned locks as it is a state that shouldn't naturally occur, so let's just remove this check
There was a problem hiding this comment.
Removed in 40d411f. The test now only asserts empty_entities_removed >= 0 since we don't actually create orphaned locks in this test scenario.
Co-authored-by: andystaples <77818326+andystaples@users.noreply.github.com>
Changes Made
test_get_orchestration_state_by_max_instance_count: Now creates exactly 3 orchestrations before querying with
max_instance_count=2, ensuring the test verifies the backend respects the limit rather than just returning all available orchestrations.test_clean_entity_storage: Switched from function-based entity to class-based
DurableEntitywhich has a built-indelete()operation, making the test cleaner and more idiomatic.Removed orphaned locks check: The test doesn't actually create orphaned locks, so removed the assertion for
orphaned_locks_releasedto be more accurate about what the test validates.✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.