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

Return an error from BrowserContext.SetExtraHTTPHeaders and handle panic in mapping layer #796

Merged
merged 2 commits into from Feb 27, 2023

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Feb 27, 2023

At the moment the codebase uses panic too liberally and we don't have control over when we actually want to abort all test runs vs when we only want to fail the current test iteration.

The idea is that we will bubble up errors all the way to the mapping layer, where it can decide on whether to panic or to return an error to goja.

This would mean that we have more control over when to panic, and to be more aware of what we want to panic on, as well as using good GO error handling practices throughout the codebase.

This is the first PR that demonstrates how we can catch internal errors. There is another PR where we had most of the discussion and several other issues were found and resolved (#744). #744 was abandoned in favour of a new PR as the previous issue would require a lot of rework.

The script i've used to test this is:

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

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

  try {
    await page.goto('https://test.k6.io/flip_coin.php')
  } finally {
    page.close()
    browser.close()
  }
}

The output of this when ran with this should be:

panic: GoError: BrowserContext.setExtraHTTPHeaders(headers) has not been implemented yet: internal error

@ankur22 ankur22 added this to the v0.9.0 milestone Feb 27, 2023
@ankur22 ankur22 self-assigned this Feb 27, 2023
@ankur22 ankur22 marked this pull request as draft February 27, 2023 10:36
@ankur22 ankur22 marked this pull request as ready for review February 27, 2023 11:19
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.

LGTM 👍

This internal error type will help us track internal errors so that
we can panic at the goja mapping layer instead of panicking from
within the business logic code.
This refactoring will catch ErrInternal errors and panic if found.
@inancgumus inancgumus added the mapping k6 browser to Goja mapping related. label Feb 27, 2023
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.

LGTM.

@ankur22 ankur22 merged commit 3a6a9ee into main Feb 27, 2023
@ankur22 ankur22 deleted the internal-error branch February 27, 2023 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mapping k6 browser to Goja mapping related. refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants