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

[YUNIKORN-2635] test coverage improvement: same priority case in sorter #871

Closed
wants to merge 2 commits into from

Conversation

0yukali0
Copy link
Contributor

What is this PR for?

Update priority cases to improve test coverage.

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring
  • - Test

Todos

  • - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2635

How should this be tested?

make test

Screenshots (if appropriate)

Questions:

  • - The licenses files need update.
  • - There is breaking changes for older versions.
  • - It needs documentation.

@0yukali0 0yukali0 self-assigned this May 19, 2024
Copy link

codecov bot commented May 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.13%. Comparing base (63a91f6) to head (4b8a355).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #871      +/-   ##
==========================================
+ Coverage   77.08%   77.13%   +0.04%     
==========================================
  Files          97       97              
  Lines       11989    11992       +3     
==========================================
+ Hits         9242     9250       +8     
+ Misses       2415     2411       -4     
+ Partials      332      331       -1     

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

@pbacsko pbacsko self-requested a review May 23, 2024 15:25
Comment on lines 302 to 303
// apps should come back in order: 0, 2, 3, 1
assertAppList(t, list, []int{0, 3, 1, 2}, "app-1 & app-3 allocated, app-3 high priority")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: comment is inconsistent with the code

input["app-2"].SubmissionTime = input["app-3"].SubmissionTime
input["app-1"].SubmissionTime = input["app-3"].SubmissionTime
list = sortApplications(input, policies.FifoSortPolicy, false, nil)
assertAppList(t, list, []int{0, 2, 3, 1}, "fifo first, priority second") // 0, 3, 1, 2
Copy link
Contributor

@pbacsko pbacsko May 23, 2024

Choose a reason for hiding this comment

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

Comment vs code: 0,3,1,2 or 0,2,3,1? I'm not sure that we need a comment at all if it's obvious.

Copy link
Contributor

@pbacsko pbacsko left a comment

Choose a reason for hiding this comment

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

+1

@pbacsko pbacsko closed this in bc56816 May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants