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

Rename ErrInternal to ErrFatal and use abort #810

Merged
merged 7 commits into from Mar 6, 2023
Merged

Rename ErrInternal to ErrFatal and use abort #810

merged 7 commits into from Mar 6, 2023

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Mar 6, 2023

Fatal better describes the error, and it will help distinguish from other internal errors (which are generally temporary). Fatal is a permanent error, which should end the whole test run not just the current iteration. If we ran the same iteration again then there's a 100% chance that we will run into the same error, e.g. trying to work with an unimplemented API is a Fatal error.

Here are two test scripts I used to test this change:

When ran with xk6 run -i 10 examples/test.js before the change, this test will iterate 10 times, failing each iteration. After the change it will stop on the first iteration.

import { chromium } from 'k6/x/browser'

export default async function () {
  const browser = chromium.launch({headless: false})
  const context = browser.newContext()
  context.cookies()
}

This test will stop the whole test run before the change and after the change.

import { chromium } from 'k6/x/browser'

export default async function () {
  const browser = chromium.launch({headless: false})
  const context = browser.newContext()
  context.setExtraHTTPHeaders()
}

Fatal better describes the error, and it will help distinguish from
other internal errors (which could be seen as temporary). Fatal is a
permanent error, which should end the whole test run not just the
current iteration.
Copy link
Collaborator

@ka3de ka3de left a comment

Choose a reason for hiding this comment

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

Thanks for refactoring this after initial discussions @ankur22 ! 👍
I added just one question/comment.

browser/mapping.go Outdated Show resolved Hide resolved
The shared code will be used for the abort method.
Panic needs to differentiate what happens when Panic is called,
vs what will happen when abort is implemented. The rest of the
sharedPanic code is needed by both.
This is to show that the whole test run will also stop when we try
to use an unimplemented unpromisified API.
Copy link
Collaborator

@ka3de ka3de left a comment

Choose a reason for hiding this comment

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

Some minor comments. Feel free to accept or discard at your discretion.
LGTM.

k6ext/panic.go Outdated Show resolved Hide resolved
k6ext/panic.go Outdated Show resolved Hide resolved
k6ext/panic.go Outdated Show resolved Hide resolved
k6ext/panic.go Outdated Show resolved Hide resolved
Cleanup `failFunc` to make it easier to read.

Co-authored-by: ka3de <daniel.jimenez@grafana.com>
@ankur22 ankur22 changed the title Rename ErrInternal to ErrFatal Rename ErrInternal to ErrFatal and use abort Mar 6, 2023
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

Nice job 👏

@ankur22 ankur22 merged commit 48ee41a into main Mar 6, 2023
@ankur22 ankur22 deleted the fatal branch March 6, 2023 16:32
@ka3de ka3de mentioned this pull request May 10, 2023
3 tasks
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.

None yet

3 participants