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

Add Tests for Jobs and Client #2725

Merged
merged 7 commits into from
Apr 12, 2023
Merged

Add Tests for Jobs and Client #2725

merged 7 commits into from
Apr 12, 2023

Conversation

ccfromms
Copy link
Contributor

@ccfromms ccfromms commented Aug 3, 2022

Description

While testing importOrchestratorJob, I discovered several issues in job tests. Essentially the issues are as following:

  • Need more tests for OrchestratorJob such as testing different job status combinations.
  • The function GetJobByGroupId in TestQueueClient only read jobInfo from local variable.
  • Missing test when job was cancelled by user and job was cancelled by itself at the same time, it is better to verify the job status in this situation.

This PR will add several tests for OrchestratorJob and SqlQueueClient, and upgrade the function for TestQueueClient.

Testing

  • Added some tests for OrchestratorJob:
    • Verify the thrown exception when some jobs cancelled or failed.
    • Verify the thrown exception and job status when job was cancelled or failed after several calls.
    • Verify the job status when job was cancelled or failed and other jobs were running or created.
  • Upgraded TestQueueClient, added func for GetJobByGroupId.
  • Added test for SqlQueueClientTest to verify job status after cancelJobByGroupId called twice.

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 50 characters
  • Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
  • Tag the PR with the type of update: Bug, Dependencies, Enhancement, or New-Feature
  • Tag the PR with Azure API for FHIR if this will release to the Azure API for FHIR managed service (CosmosDB or common code related to service)
  • Tag the PR with Azure Healthcare APIs if this will release to the Azure Healthcare APIs managed service (Sql server or common code related to service)
  • CI is green before merge
  • Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

@ccfromms ccfromms added the Enhancement-Test Enhancement on tests. label Aug 3, 2022
@ccfromms ccfromms added this to the S94 milestone Aug 3, 2022
tongwu-sh
tongwu-sh reviewed Aug 3, 2022
View reviewed changes
test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlQueueClientTests.cs Outdated
@@ -259,6 +260,40 @@ public async Task GivenGroupJobs_WhenCancelJobsByGroupId_ThenAllJobsShouldBeCanc
Assert.Equal(jobInfo2.Result, jobInfo.Result);
}

[Fact]
public async Task GivenGroupJobs_WhenCancelJobsByGroupIdCalledTwoTimes_ThenJobStatusShouldNotChange()
Copy link
Contributor

@tongwu-sh tongwu-sh Aug 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

TwoTimes

Twice？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I will modify the name of the function.

tongwu-sh
tongwu-sh reviewed Aug 3, 2022
View reviewed changes
test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlQueueClientTests.cs Outdated
await sqlQueueClient.CancelJobByGroupIdAsync(queueType, jobInfo2.GroupId, CancellationToken.None);
Assert.True((await sqlQueueClient.GetJobByGroupIdAsync(queueType, jobInfo2.GroupId, false, CancellationToken.None)).All(t => t.Status == JobStatus.Cancelled || t.Status == JobStatus.Failed || (t.Status == JobStatus.Running && t.CancelRequested)));
jobInfo1 = await sqlQueueClient.GetJobByIdAsync(queueType, 1, false, CancellationToken.None);
jobInfo2 = await sqlQueueClient.GetJobByIdAsync(queueType, 2, false, CancellationToken.None);
Copy link
Contributor

Choose a reason for hiding this comment

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

2

dequeue operation do not guarantee

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I will check and replace number with variable.

@fhibf
Copy link
Contributor

fhibf commented Oct 5, 2022

Hi! Please, sync this PR with main.
I've added new validations on top of tests, requiring from now on at least two traits per test class: a Category and the Owning Team.

@ccfromms
Copy link
Contributor Author

ccfromms commented Oct 6, 2022

Hi! Please, sync this PR with main. I've added new validations on top of tests, requiring from now on at least two traits per test class: a Category and the Owning Team.

Ok, I will sync this PR.

@rbans96
Copy link
Contributor

rbans96 commented Oct 11, 2022

@ccfromms Thanks for syncing with main :) I am wondering if this PR is still in draft or can be published as 'Ready for review'?

@ccfromms ccfromms marked this pull request as ready for review October 12, 2022 03:29
@ccfromms ccfromms requested a review from a team as a code owner October 12, 2022 03:29
@ccfromms
Copy link
Contributor Author

@ccfromms Thanks for syncing with main :) I am wondering if this PR is still in draft or can be published as 'Ready for review'?

Yes, this PR is ready for review. I have turned it open. Thanks!

@brendankowitz
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@brendankowitz
Copy link
Member

@ccfromms would you be able to update with main again?

@ccfromms
Copy link
Contributor Author

ccfromms commented Nov 2, 2022

@ccfromms would you be able to update with main again?

Ok, I will update it.

@LTA-Thinking
Copy link
Collaborator

This PR needs to be reviewed and probably updated. @EXPEkesheth I'll make a small story for next sprint to review this and get it merged.

@LTA-Thinking LTA-Thinking added the Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs label Apr 5, 2023
@LTA-Thinking LTA-Thinking merged commit 5dcdcf0 into main Apr 12, 2023
@LTA-Thinking LTA-Thinking deleted the personal/chen/tests branch April 12, 2023 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tongwu-sh tongwu-sh tongwu-sh left review comments

@PTaladay PTaladay PTaladay approved these changes

Assignees
No one assigned
Labels
Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Enhancement-Test Enhancement on tests.
Projects
None yet
Milestone
S94
Development

Successfully merging this pull request may close these issues.
7 participants
@ccfromms @fhibf @rbans96 @brendankowitz @LTA-Thinking @PTaladay @tongwu-sh