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

Fix an incorrect outcome (corner cases) #917

Merged
merged 11 commits into from
Aug 5, 2020
Merged

Conversation

pawelpasterz
Copy link
Contributor

@pawelpasterz pawelpasterz commented Jul 30, 2020

Fixes #914

Due to the incorrectly implemented logic regarding the status update, there were cases when flaky tests obscure failed results.
The following must occur:

  1. The matrix needs to have both flaky and failed tests (at least one of each)
  2. TestExecutions need to be returned in a specific order:
    1. Failed execution must be consumed by flank before flaky one
    2. The following flaky execution must be actually failed but the outcome of all re-runs is success
  3. Then flank does what is the following:
    1. The outcome is set to failure when failed test/shards/executions are processed
    private fun Outcome?.updateOutcome(
       when {
           ...
           else -> this?.summary  // at this point SavedMatrix.outcome == success, flank changes it to failure
           ...
    })
    1. When flank reaches case where step summary is failure but execution is success it sets SavedMatrix outcome to flaky
    updateOutcome(flakyOutcome = it.step.outcome?.summary != this?.summary) // flakyOutcome == true 
    1. Due to incorrect order in when condition failure check is never reached
    private fun Outcome?.updateOutcome(
           flakyOutcome: Boolean
       ) {
           outcome = when {
               flakyOutcome -> flaky 
               // flank should escape here with failure status persisted, but since flakyOutcome == true SavedMatrix.outcome is changed to flaky
               outcome == failure || outcome == inconclusive -> return 
               outcome == flaky -> this?.summary?.takeIf { it == failure || it == inconclusive }
               else -> this?.summary
           } ?: outcome
       }

Proposed fix

  1. Change order in when condition to always check for failure first
    private fun Outcome?.updateOutcome(
           flakyOutcome: Boolean
       ) {
           outcome = when {
               outcome == failure || outcome == inconclusive -> return // escape when failure/inconclusive outcome is set
               flakyOutcome -> flaky 
               outcome == flaky -> this?.summary?.takeIf { it == failure || it == inconclusive }
               else -> this?.summary
           } ?: outcome
       }
  2. Add unit tests to cover this case.

Test Plan

How do we know the code works?

All tests pass and build finish successfully.
No easy way to verify it without previously prepared data/results

Checklist

  • Documented
  • Unit tested
  • release_notes.md updated

@pawelpasterz pawelpasterz self-assigned this Jul 30, 2020
@pawelpasterz pawelpasterz marked this pull request as ready for review July 31, 2020 13:39
@pawelpasterz pawelpasterz changed the title Fix incorrect outcome (corner cases) Fix an incorrect outcome (corner cases) Jul 31, 2020
}

@Test
fun `savedMatrix should have failed outcome when at least one test is failed and the last one is flaky`() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fun `savedMatrix should have failed outcome when at least one test is failed and the last one is flaky`() {
fun `savedMatrix should have failed outcome when at least one test is failed`() {

@pawelpasterz pawelpasterz force-pushed the fix-incorrect-outcome branch from 559904e to 61c2dbf Compare July 31, 2020 14:01
@pawelpasterz pawelpasterz requested a review from jan-goral July 31, 2020 18:52
piotradamczyk5
piotradamczyk5 previously approved these changes Aug 3, 2020
adamfilipow92
adamfilipow92 previously approved these changes Aug 3, 2020
jan-goral
jan-goral previously approved these changes Aug 3, 2020
adamfilipow92
adamfilipow92 previously approved these changes Aug 3, 2020
piotradamczyk5
piotradamczyk5 previously approved these changes Aug 4, 2020
@pawelpasterz pawelpasterz force-pushed the fix-incorrect-outcome branch from 6cc5607 to f978d30 Compare August 5, 2020 11:36
@pawelpasterz pawelpasterz merged commit d5edb88 into master Aug 5, 2020
@pawelpasterz pawelpasterz deleted the fix-incorrect-outcome branch August 5, 2020 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Outcome incorrectly set by Flank
4 participants