fix: Release metrics return incorrect featuresPerWeek value #280#306
Conversation
|
So if there no bug – then we can't create any dataset from this issue, we just cancel it. No need to create a PR next time – you can just drop a comment on the issue 👍 |
|
So actually.... this isn't a problem with the test, the bug is valid! Your test mocks the value "now" to comply with the logic, but that is exactly the problem: logic of calculating this percentage of features per week should not depend on the current time for the releases that are already released! This is exactly the whole point: when a release already released, this value shouldn't change, no matter how much time has passed since then. Look at the golden test referred on the bug issue: it actually emulates the correct behaviour for a releases that has released_at date, and not mocking any instants. |
innokenty
left a comment
innokenty
left a comment
There was a problem hiding this comment.
The bug-issue is valid: please re-analyze, add a bugfix and provide correct tests that validate the logic correctly. Look into the linked golden test – it seems pretty good to me.
FAIL_TO_PASS: MetricsIntegrationTests#shouldCalculateFeaturesPerWeekExactlyForKnownFixture, MetricsIntegrationTest#shouldReturnZeroVelocityWhenLessThanTwoWeeksOfData, MetricsIntegrationTest#shouldReturnZeroVelocityWhenThereIsNoData
YuliaMittova
force-pushed
the
bugfix/280-release-metrics-return-incorrect-featuresPerWeek-value
branch
from
efe0e29 to
7015624
Compare
@innokenty fixed logic and tests. Please take a look. |
| @Test | ||
| void shouldCalculateFeaturesPerWeekExactlyForKnownFixture() throws Exception { | ||
| assertThat(d(velocity(getMetrics("TEST-FAST")), "featuresPerWeek")).isEqualTo(5.0); | ||
| } | ||
|
|
||
| @Test | ||
| void shouldReturnZeroVelocityWhenLessThanTwoWeeksOfData() throws Exception { | ||
| assertThat(d(velocity(getMetrics("GO-2024.2.3")), "featuresPerWeek")).isEqualTo(0.0); | ||
| } | ||
|
|
||
| @Test | ||
| void shouldReturnZeroVelocityWhenThereIsNoData() throws Exception { | ||
| assertThat(d(velocity(getMetrics("GO-2024.2.7")), "featuresPerWeek")).isEqualTo(0.0); | ||
| } |
There was a problem hiding this comment.
Please add more test coverage. The only real test is only checking the most simple case of "5.0" and does not cover all the use cases.
innokenty
left a comment
innokenty
left a comment
There was a problem hiding this comment.
We need a bit more tests, than a single basic happy-case scenario, please cover some corner cases.
|
|
||
| private long calculateBusinessWeeksElapsed(Instant startDate) { | ||
| long businessDaysElapsed = calculateBusinessDaysElapsed(startDate); | ||
| private long calculateBusinessWeeksElapsed(Instant startDate, Instant endDate) { |
There was a problem hiding this comment.
The precision is lost here: long businessDaysElapsed / 5.
Please preserve the fractional business-week value instead of truncating it, and add a test where the period between createdAt and releasedAt includes a partial business week.
There was a problem hiding this comment.
yeah, we definitely need a test for a fractional value
3 participants
Bugfix for issue #280