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

feat(runner): enable the bail setting #7942

Merged
merged 3 commits into from
Sep 20, 2024
Merged

Conversation

ihexxa
Copy link
Contributor

@ihexxa ihexxa commented Sep 11, 2024

Background

By enabling "stop on error" setting, runner stops immediately when it sees an error.

Changes

  • Enable the "bail" setting
  • Minor style fixes

Ref: INS-4403

@ihexxa ihexxa self-assigned this Sep 11, 2024
@ihexxa ihexxa changed the title feat/(runner): enable the stop-on-err setting feat(runner): enable the stop-on-err setting Sep 11, 2024
@ihexxa ihexxa marked this pull request as draft September 11, 2024 07:24
@ihexxa ihexxa force-pushed the feat/settings-stop-on-err branch 2 times, most recently from 292975c to f1c5b4f Compare September 12, 2024 02:28
@ihexxa ihexxa marked this pull request as ready for review September 12, 2024 11:34
@jackkav
Copy link
Contributor

jackkav commented Sep 12, 2024

in the CLI and other testing frameworks this is referred to as "bail"

also please avoid abbreviation like "curIteration" where possible.

@ihexxa
Copy link
Contributor Author

ihexxa commented Sep 13, 2024

Let me update that part although I just copied them. 🥹

@ihexxa ihexxa force-pushed the feat/settings-stop-on-err branch from c37ca27 to a91d08a Compare September 14, 2024 01:32
@ihexxa ihexxa changed the title feat(runner): enable the stop-on-err setting feat(runner): enable the bail setting Sep 14, 2024
@ihexxa ihexxa requested a review from a team September 14, 2024 06:50
Copy link
Member

@filfreire filfreire left a comment

Choose a reason for hiding this comment

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

@ihexxa I've tried

  • setting an assertion of a request to fail
  • causing a reference error (e.g. non existing variable)
  • or adding a non existing environment variable / template tag to the url path

in none of these cases did the bail seem to work, still executing the request that came afterwards.

Is there any other type of errors that are actually meant to make the option trigger?

@ihexxa
Copy link
Contributor Author

ihexxa commented Sep 18, 2024

Thanks @filfreire let me check, assertion failure and non-exist variables might not be regarded as "error"s in runner. You could try just throw an error although it is a bit wild.

@ihexxa ihexxa force-pushed the feat/settings-stop-on-err branch from a91d08a to 3a8030a Compare September 19, 2024 04:15
@notjaywu
Copy link
Contributor

@ihexxa I've tried

  • setting an assertion of a request to fail
  • causing a reference error (e.g. non existing variable)
  • or adding a non existing environment variable / template tag to the url path

in none of these cases did the bail seem to work, still executing the request that came afterwards.

Is there any other type of errors that are actually meant to make the option trigger?

From my point of view, setting an assertion of a request to fail is more like a feature request, some others may want the progress to continue.

@ihexxa ihexxa force-pushed the feat/settings-stop-on-err branch from 3a8030a to 3e1d14b Compare September 20, 2024 03:18
@jackkav
Copy link
Contributor

jackkav commented Sep 20, 2024

bail or stop-on-error, should exit the runner if at any point a test fails or an unhandled exception occurs, in my mental modal a request is equivalent to a test suite containing 0..n tests. In the cli i need to keep a success flag in the outer loop and return out oif the runner loop early if bail is true. In this PR a throw is probably a more sensible way to bail.

Copy link
Member

@filfreire filfreire left a comment

Choose a reason for hiding this comment

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

LGTM

Bail should now work for referenceError thrown - I think this is OK 👍

Assertion and missing Env variable - might not be be a good idea to add now

We found in other clients the behavior is the same.

There is one extra case where an assertion can fail - and what's the behavior of that failure when outside of a test's scope - but it should be ok to leave it out of scope of this PR

@ihexxa ihexxa force-pushed the feat/settings-stop-on-err branch from c4e9528 to 6806665 Compare September 20, 2024 09:04
@ihexxa ihexxa enabled auto-merge (squash) September 20, 2024 09:07
@ihexxa ihexxa merged commit 8ab0b91 into develop Sep 20, 2024
8 checks passed
@ihexxa ihexxa deleted the feat/settings-stop-on-err branch September 20, 2024 09:15
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.

4 participants