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.

Sign up for GitHub

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

Report ThresholdsHaveFailed on Cloud runs #3876

Merged
merged 2 commits into from
Aug 22, 2024
Merged

Report ThresholdsHaveFailed on Cloud runs #3876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e031507
Report ThresholdsHaveFailed on Cloud runs
joanlopez Jul 30, 2024
2830884
Merge branch 'master' into cloud-thresholds
joanlopez Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions cmd/cloud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

"github.com/fatih/color"
"go.k6.io/k6/cloudapi"
"go.k6.io/k6/cmd/state"
"go.k6.io/k6/errext"
"go.k6.io/k6/errext/exitcodes"
"go.k6.io/k6/lib"
Expand All @@ -22,8 +23,6 @@ import (

"github.com/spf13/cobra"
"github.com/spf13/pflag"

"go.k6.io/k6/cmd/state"
)

// cmdCloud handles the `k6 cloud` sub-command
Expand Down Expand Up @@ -313,9 +312,16 @@ func (c *cmdCloud) run(cmd *cobra.Command, args []string) error {
logger.WithField("run_status", testProgress.RunStatusText).Debug("Test finished")
}

//nolint:stylecheck,golint
if testProgress.ResultStatus == cloudapi.ResultStatusFailed {
// Although by looking at [ResultStatus] and [RunStatus] isn't self-explanatory,
// the scenario when the test run has finished, but it failed is an exceptional case for those situations
// when thresholds have been crossed (failed). So, we report this situation as such.
if testProgress.RunStatus == cloudapi.RunStatusFinished {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not certain about this assumption. During test run it possible also that an error during execution will happen and or it could be aborted by limits, like the comment below stays, so just being in a run state doesn't guaranty that the test has been aborted by thresholds.

However, I must admit I don't know how does the response from backend look like, have you chatted with them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I got it correctly, we have RunStatuses specifically for those situations (see https://github.com/grafana/k6/blob/master/cloudapi/run_status.go#L16-L20).

Also, my assumption is that RunStatusAbortedThreshold doesn't apply here, because in this case the test hasn't been aborted (abortOnFail: true), just finished and failed at the end, because unsuccessful threshold checks.

And yes, that's what I got after a conversation with @d14c and @Griatch, so maybe they can confirm it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additionally, I know there's other edge cases that we may want to handle within this piece of code (other of the aformentioned RunStatuses), but as I left as note in the PR description, I'd prefer to just focus on this edge case here, and leave the other scenarios for upcoming, separated PRs.

Copy link
Contributor

@codebien codebien Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with @olegbespalov that sounds like a risky assumption. Furthermore, I have the feeling that all the time that we need to read this code, it will required a lot of cognitive load.

Could we ask the backend to add a dedicated RunStatus for it? Like: RunStatusFinishedWithCrossedThresholds.

Copy link

@Griatch Griatch Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@joanlopez A few points here:

  • ABORTED_BY_THRESHOLD this is a run_status end status for thresholds that are set to abort the test when crossed. In this case the test will never reach run_status FINISHED.
  • run_status FINISHED can still be reached if the threshold was not set up to abort the test. In this case the outcome will only be reflected in the result_status which is handled completely separately from run_status.
  • result_status False/True will start out as True and will switch to False only on a failed threshold afaik. So it's for example possible to have a test that has a run_status ABORTED_SYSTEM but with result_status True because the threshold was never crossed.

For the upcoming public API we are introducing a 'success' status that is a little easier to reason around. Inviting @fornfrey on this to make sure I get it right; but it's basically a combination of run/result status that will fail both on a threshold-cross and on a test that fail to complete properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, if I got it right, it looks like my assumptions are confirmed again by @Griatch.

What do you think guys? @olegbespalov @codebien
Do you want to go like this? Or wait until we have the new API with clearer information?

If we prefer to wait, I can close this PR, and create an issue with some context, and marked as blocked, waiting for the new public API. As you prefer!

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for all input! it sounds all clear and ledgit and it seems like we could clean the TODO

// TODO: use different exit codes for failed thresholds vs failed test (e.g. aborted by system/limit)

If we prefer to wait, I can close this PR, and create an issue with some context, and marked as blocked, waiting for the new public API. As you prefer!

I don't know about ETA of new API, but if we could bring value earlier that sounds better.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess my question is how likely it is:

  • that the test has finished
  • fails
  • and it isn't a threshold

I guess it is fine if we have some false possitives, but still.

Copy link
Contributor Author

@joanlopez joanlopez Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't that what previous comment from @Griatch explains? Like:

result_status False/True will start out as True and will switch to False only on a failed threshold afaik.

I think that, in case there's any other error (e.g. aborted by system), that's reflected on the run_status, and not on the result_status, which seems to be tied to thresholds.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, I guess I did skim over that particular line a bit too much.

Again this seems like a strange behaviour, but I guess it is what it is .

return errext.WithExitCodeIfNone(errors.New("Thresholds have been crossed"), exitcodes.ThresholdsHaveFailed)
}

// TODO: use different exit codes for failed thresholds vs failed test (e.g. aborted by system/limit)
//nolint:stylecheck,golint
return errext.WithExitCodeIfNone(errors.New("The test has failed"), exitcodes.CloudTestRunFailed)
}

Expand Down
29 changes: 25 additions & 4 deletions cmd/tests/cmd_cloud_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@ import (
"path/filepath"
"testing"

"go.k6.io/k6/lib/testutils"

"go.k6.io/k6/cloudapi"
"go.k6.io/k6/cmd"
"go.k6.io/k6/errext/exitcodes"
"go.k6.io/k6/lib/fsext"
"go.k6.io/k6/lib/testutils"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.k6.io/k6/cloudapi"
"go.k6.io/k6/lib/fsext"
)

func TestK6Cloud(t *testing.T) {
Expand Down Expand Up @@ -236,6 +236,27 @@ func runCloudTests(t *testing.T, setupCmd setupCommandFunc) {
assert.Contains(t, stdout, `output: https://app.k6.io/runs/123`)
assert.Contains(t, stdout, `test status: Finished`)
})

t.Run("TestCloudThresholdsHaveFailed", func(t *testing.T) {
t.Parallel()

progressCallback := func() cloudapi.TestProgressResponse {
return cloudapi.TestProgressResponse{
RunStatusText: "Finished",
RunStatus: cloudapi.RunStatusFinished,
ResultStatus: cloudapi.ResultStatusFailed,
Progress: 1.0,
}
}
ts := getSimpleCloudTestState(t, nil, setupCmd, nil, nil, progressCallback)
ts.ExpectedExitCode = int(exitcodes.ThresholdsHaveFailed)

cmd.ExecuteWithGlobalState(ts.GlobalState)

stdout := ts.Stdout.String()
t.Log(stdout)
assert.Contains(t, stdout, `Thresholds have been crossed`)
})
}

func cloudTestStartSimple(tb testing.TB, testRunID int) http.Handler {
Expand Down