You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by "SimonSimonB (via GitHub)" <gi...@apache.org> on 2023/05/26 13:05:23 UTC
[GitHub] [yunikorn-core] SimonSimonB opened a new pull request, #555: [YUNIKORN-1403] Add comment to explain lack of test coverage
SimonSimonB opened a new pull request, #555:
URL: https://github.com/apache/yunikorn-core/pull/555
### What is this PR for?
[YUNIKORN-1403](https://issues.apache.org/jira/browse/YUNIKORN-1403) asks for a test so that [this code](https://github.com/apache/yunikorn-core/blob/master/pkg/common/resources/resources.go#L965-L972) handling a negative overflow, which is currently not covered by the tests, is tested. In fact, there already exists [a test](https://github.com/apache/yunikorn-core/blob/master/pkg/common/resources/resources_test.go#L1512-L1516) that is meant to exercise that code. As far as I understand, the reason that this test does not exercise the code is that the [type conversion](https://github.com/apache/yunikorn-core/blob/master/pkg/common/resources/resources.go#L956) from `float64` to `int64` does not lead to negative overflows (at least not for the values that I tried!), although it does lead to positive overflows. This is demonstrated by the following Go program:
```go
package main
import (
"fmt"
"math"
)
func main() {
tooSmall := float64(math.MinInt64) * 100
tooBig := float64(math.MaxInt64) * 100
fmt.Printf("tooSmall < math.MinInt64: %t\n", tooSmall < math.MinInt64)
fmt.Printf("int64(tooSmall) > 0: %t\n", int64(tooSmall) > 0)
fmt.Printf("tooBig > math.MaxInt64: %t\n", tooBig > math.MaxInt64)
fmt.Printf("int64(tooBig) < 0: %t\n", int64(tooBig) < 0)
}
```
which prints
```
tooSmall < math.MinInt64: true
int64(tooSmall) > 0: false
tooBig > math.MaxInt64: true
int64(tooBig) < 0: true
```
in Go 1.20. This behaviour does not violate the [Go spec](https://go.dev/ref/spec#Conversions), which leaves the behaviour in this case undefined:
> In all non-constant conversions involving floating-point or complex values, if the result type cannot represent the value the conversion succeeds but the result value is implementation-dependent.
So, the test currently does not exercise the code it wants to exercise, but it may do so in future versions of Go.
My sense is that it would be best to 1) keep the test in case future implementations lead to negative overflows, 2) add a comment above the uncovered code to explain why tests may not exercise this code in some versions of Go. This PR adds the explanatory comment.
(Someone else self-assigned the JIRA issue back in November. Since this is a small issue and no activity was recorded since November, I assumed it would be fine to open a PR addressing it. I hope that was okay.)
### What type of PR is it?
* [ ] - Bug Fix
* [ ] - Improvement
* [ ] - Feature
* [X] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
### What is the Jira issue?
[YUNIKORN-1403](https://issues.apache.org/jira/browse/YUNIKORN-1403)
### How should this be tested?
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] wilfred-s commented on pull request #555: [YUNIKORN-1403] Add comment to explain lack of test coverage
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s commented on PR #555:
URL: https://github.com/apache/yunikorn-core/pull/555#issuecomment-1567685523
See comment in the jira. This is not correct
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] wilfred-s closed pull request #555: [YUNIKORN-1403] Improve overflow handling
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s closed pull request #555: [YUNIKORN-1403] Improve overflow handling
URL: https://github.com/apache/yunikorn-core/pull/555
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] codecov[bot] commented on pull request #555: [YUNIKORN-1403] Add comment to explain lack of test coverage
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #555:
URL: https://github.com/apache/yunikorn-core/pull/555#issuecomment-1564808777
## [Codecov](https://app.codecov.io/gh/apache/yunikorn-core/pull/555?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#555](https://app.codecov.io/gh/apache/yunikorn-core/pull/555?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (f529143) into [master](https://app.codecov.io/gh/apache/yunikorn-core/commit/c5d26a841d1f9e240620ffa6d8b805b163512bbc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (c5d26a8) will **decrease** coverage by `0.02%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #555 +/- ##
==========================================
- Coverage 74.84% 74.82% -0.02%
==========================================
Files 70 70
Lines 11523 11526 +3
==========================================
Hits 8624 8624
- Misses 2595 2598 +3
Partials 304 304
```
| [Impacted Files](https://app.codecov.io/gh/apache/yunikorn-core/pull/555?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [pkg/common/resources/resources.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/555?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `91.78% <0.00%> (-0.46%)` | :arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] SimonSimonB commented on pull request #555: [YUNIKORN-1403] Add comment to explain lack of test coverage
Posted by "SimonSimonB (via GitHub)" <gi...@apache.org>.
SimonSimonB commented on PR #555:
URL: https://github.com/apache/yunikorn-core/pull/555#issuecomment-1567777443
Thanks for the speedy review, @wilfred-s! I'll take a look at the values you tried in the comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org