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

Option 'throw' is broken or badly documented #251

Closed
marklagendijk opened this issue Sep 26, 2019 · 8 comments
Closed

Option 'throw' is broken or badly documented #251

marklagendijk opened this issue Sep 26, 2019 · 8 comments
Labels
Area: OSS Content Improvements or additions to community/oss documentation

Comments

@marklagendijk
Copy link

Expected behaviour

Enabling throw exits the specific test iteration as soon as an request error occurs: connection issues, status 4xx, status 5xxx.

I hoped that with this option I wouldn't have to write checks for every request.

Actual behaviour

Status errors don't exit the iteration, or show anything in the log.

Documented behaviour

A boolean, true or false, specifying whether to throw errors on failed HTTP requests or not.

@cuonglm
Copy link

cuonglm commented Sep 27, 2019

It depends on how you defined failed request.

You can load test an endpoint which return 404 or 400. If you expect it to return 400, but it return 200, then it's a failed request in scope of your test.

@marklagendijk
Copy link
Author

In the end I assumed that that is indeed what the documentation meant.
However, that is not what I, as a test developer, would assume it means when reading the documentation.

Apart from the documentation I also think that the feature would make much more sense when it also applied to 4xx and 5xx errors:

  • In most tests every request should return a success status 2xx.
  • In load tests it can happen that one overloads the system. In such case you usually get a 503 error.
  • If the feature would also cover error statuses, you could just enable it, without having to worry about writing a check for every request.

@cuonglm
Copy link

cuonglm commented Sep 27, 2019

@markjmeier I looked at the source code again, and the term of failed request is not about 4xx and 5xx status code, but k6 can't connect to remote host.

	if resErr != nil {
		// Do *not* log errors about the contex being cancelled.
		select {
		case <-ctx.Done():
		default:
			state.Logger.WithField("error", resErr).Warn("Request Failed")
		}

		if preq.Throw {
			return nil, resErr
		}
	}```

@na--
Copy link
Member

na-- commented Sep 27, 2019

Thanks for mentioning this, we'll improve the docs for throw.

But the use case of throwing errors when the connection can't be established (or there's a DNS error, TLS error, etc. non-application errors) or when the remote server returns a response with a 4xx/5xx (application error) status is a very valid use case. Unfortunately, the current global throw option is too inflexible, so I'm thinking that this is also something we should handle in the new HTTP API, so I'll link it that new issue.

@na-- na-- transferred this issue from grafana/k6 Apr 1, 2021
@simskij
Copy link
Contributor

simskij commented Jun 11, 2021

@na-- should this really be here? I get the impression that you wanted to have this as some kind of input to the design of the new HTTP API?

@simskij simskij added Area: OSS Content Improvements or additions to community/oss documentation Priority: Low Status: Triage labels Jun 11, 2021
@na--
Copy link
Member

na-- commented Jun 11, 2021

A bit of both. We should better document how the current throw option works, because it's unfortunately not going to disappear anytime soon and it has some behaviors that, while mostly logically consistent, are still somewhat surprising. For example, the fact that it won't throw an error for a 4xx or 5xx HTTP response, but will for a network failure.

Or that, until recently, if you had mistyped your URL, k6 would have thrown an error, even if throw was false. This was a bug and has now fixed in grafana/k6#2045 and will be released in k6 v0.33.0 at the end of the month, but it probably still deserves a mention, since the options page wasn't part of the docs versioning done in #283.

And yes, we'll probably rethink if we'll even have a throw option in that new eventual HTTP API. But that has been on the back of the backlog for at least 2 years now, and it may not land any time soon, even in an xk6 extension... 😞 And even when it lands, we probably will still have to support the throw option for a while longer in the old API, just for backwards compatibility...

In summary: this issue should be here, and it probably deserves at least a normal priority, since it's a common footgun.

@simskij
Copy link
Contributor

simskij commented Jun 11, 2021

Appreciated, thanks!

@ppcano
Copy link
Collaborator

ppcano commented Oct 31, 2022

It has been three years since the creation of this issue.

Now, k6chaijs tries to solve this problem by throwing an assertion on failures.

I am closing this issue. I suggest following the discussion on the k6 repo or https://github.com/grafana/k6-jslib-k6chaijs/

@ppcano ppcano closed this as completed Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OSS Content Improvements or additions to community/oss documentation
Projects
None yet
Development

No branches or pull requests

5 participants