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

Improve test coverage of quality of service #5963

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

luckyarthur
Copy link
Contributor

@luckyarthur luckyarthur commented Nov 6, 2024

Why are the changes needed?

the test coverage of quality_of_service is 62.12%, which is far away from 80%, so I add some test cases in this PR.

What changes were proposed in this pull request?

  1. to improve the test coverage flyteadmin/pkg/manager/impl/executions/quality_of_service.go, more test cases are added.

  2. some nil pointer bugs found when add test cases, and fixed in this PR.
    more details about the nil pointer bug explanation:

image for this part, to get into this if condition `input.ExecutionCreateRequest.Spec.QualityOfService` must be nil to fail the first if condition `if input.ExecutionCreateRequest.Spec.QualityOfService != nil `, and also the code here needs LaunchPlan service info here image same reason as above to run into this condition, but here we are supposed to offer workflow service info 截屏2024-11-12 19 26 04 same reason as above to run into this condtion, but qualityOfService info is needed image to run into this if condition, `input.Workflow.Closure.CompiledWorkflow.Primary.Template.Metadata.QualityOfService` must be nil in the previous condition `if input.Workflow.Closure.CompiledWorkflow.Primary.Template.Metadata != nil && input.Workflow.Closure.CompiledWorkflow.Primary.Template.Metadata.QualityOfService != nil ` the `qualityOfService.GetTier()` should be used here

How was this patch tested?

  1. add more test cases to cover some scenarios missed
  2. all test passed when I make each commit

Check all the applicable boxes

  • [ ] I updated the documentation accordingly.
  • [ x ] All new and existing tests passed.
  • [ ] All commits are signed-off.

Copy link

welcome bot commented Nov 6, 2024

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 50.86%. Comparing base (636cc23) to head (8a2b430).
Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
.../pkg/manager/impl/executions/quality_of_service.go 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #5963       +/-   ##
===========================================
+ Coverage   36.81%   50.86%   +14.05%     
===========================================
  Files        1310     1164      -146     
  Lines      131034    89475    -41559     
===========================================
- Hits        48237    45512     -2725     
+ Misses      78611    39956    -38655     
+ Partials     4186     4007      -179     
Flag Coverage Δ
unittests-datacatalog 51.58% <ø> (ø)
unittests-flyteadmin 54.19% <75.00%> (+0.15%) ⬆️
unittests-flytecopilot 11.73% <ø> (ø)
unittests-flytectl 62.39% <ø> (ø)
unittests-flyteidl ?
unittests-flyteplugins 53.84% <ø> (+0.20%) ⬆️
unittests-flytepropeller 42.90% <ø> (-0.01%) ⬇️
unittests-flytestdlib 55.42% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Cold PRs
Development

Successfully merging this pull request may close these issues.

1 participant