ankur22
commented
1 year ago Types of panic
- ✅ go
panic. -- all documented k6ext.Panic.- ✅
k6common.Throw. -- all documented
Panic vs Error
Error
Should denote to the user that an error occurred but there's a way around it, i.e. they can fix it themselves. This shouldn't stop the whole test but just the current iteration.
Panic
Should denote an abort of the whole test run (all iterations) as an unexpected internal error was encountered which we need to fix e.g. the goja runtime is missing from the content. The assumption i'm making is that if it happens in the current iteration it's probably happening in all iterations.
Areas where I think are ok to use panic (abort the whole test run)
NOTE:
List of panics that could be errors (only stop the current test iteration)
| Where | reason |
|---|---|
| https://github.com/grafana/xk6-browser/blob/e62e7c12372f210914ea8d9ba70ddb392dbdc93e/chromium/browser_type.go#L68 | unimplemented method, user can remove the call |
| https://github.com/grafana/xk6-browser/blob/e62e7c12372f210914ea8d9ba70ddb392dbdc93e/chromium/browser_type.go#L210 | unimplemented method, user can remove the call |
Question
- Can we return an error to goja instead of panicking?
- Yes we can. I tested this with the
Launchmethod, and the behaviour seems to be the same if we panic fromLaunchor return an error. The difference is that the API changes fromLaunch(opts goja.Value) api.BrowsertoLaunch(opts goja.Value) (api.Browser, error). POC: https://github.com/grafana/xk6-browser/pull/718 - IMO a panic should be used to denote an abort of the whole test run (all iterations) as an unexpected internal error was encountered which we need to fix e.g. the goja runtime is missing from the content.
- IMO an error should denote a user error that can be fixed by the user themselves e.g. the element with name "foo" couldn't be found, and a time out occured. This shouldn't stop the whole test but just the current iteration.
- Yes we can. I tested this with the
- Panic in code is ok if running on the "
main" thread, which should mean it's ok to panic from APIs, e.g.gotosince it will only stop the current iteration.- This is true. A panic on the "
main" thread will stop the current iteration but not the whole test run.
- This is true. A panic on the "
inancgumus
Brief summary
Goroutines other than the one that drives an iteration should not panic. Otherwise, k6 won't catch the panic and fail.
We should take control of our non-iteration goroutines: Not panic and cause an iteration to be prematurely aborted.
More details about how to craft a solution:
xk6-browser version
v0.7.0
OS
Staging Cloud
Docker version and image (if applicable)
grafana/k6-cloud-scripts-to-archive:2.40.6-xk6browser-v0.7.0-1
Steps to reproduce the problem
Pawel ran the following script, and we got sentry alerts because of the panics.
This script panics.
```javascript import { check, sleep } from 'k6'; import { Counter } from 'k6/metrics'; import { chromium } from 'k6/x/browser'; const checkpoint1 = new Counter('checkpoint1'); const checkpoint2 = new Counter('checkpoint2'); const checkpoint3 = new Counter('checkpoint3'); const checkpoint4 = new Counter('checkpoint4'); const checkpoint5 = new Counter('checkpoint5'); const checkpoint6 = new Counter('checkpoint6'); const checkpoint7 = new Counter('checkpoint7'); export let options = { vus: 100, duration: '10h', } export function setup(){ console.log("Running setup") } export default function () { const browser = chromium.launch({ headless: true }); const context = browser.newContext(); const page = context.newPage(); checkpoint1.add(1); // Goto front page, find login link and click it page.goto('https://test.k6.io/', { waitUntil: 'networkidle' }); checkpoint2.add(1); Promise.all([ page.waitForNavigation(), page.locator('a[href="/my_messages.php"]').click(), ]).then(() => { checkpoint3.add(1) // Enter login credentials and login page.locator('input[name="login"]').type('admin'); page.locator('input[name="password"]').type('123'); // We expect the form submission to trigger a navigation, so to prevent a // race condition, setup a waiter concurrently while waiting for the click // to resolve. checkpoint4.add(1) return Promise.all([ page.waitForNavigation(), page.locator('input[type="submit"]').click(), ]); }).then(() => { checkpoint5.add(1) check(page, { 'header': page.locator('h2').textContent() == 'Welcome, admin!', }); }).finally(() => { checkpoint6.add(1) page.close(); browser.close(); checkpoint7.add(1) }); sleep(1); } export function teardown() { console.log("Running teardown") } ```The script has an incorrect usage of promises, and the fix is easy:
However, the problem is deeper than it seems.
See more details in the test run: 1620122 (proxy).
Actual behaviour
Panic causes an iteration to be prematurely aborted.