Chrome data store directory is not cleaned up

[imiric]

Currently (12eeeb5) the temporary Chrome data storage directory is not cleaned up after each test run.

This is especially visible when running tests:

$ find /tmp -type d -name 'xk6-browser-data-*' | wc -l
0
$ GOMAXPROCS=3 go test -p 3 -v -race -count=1 ./...
[...]
$ find /tmp -type d -name 'xk6-browser-data-*' | wc -l
40

This doesn't seem like a regression and is the way it's always worked, at least going back to v0.1.1.

Suggested solution

To avoid leaving files behind, the extension should by default remove this directory when the test is finished. This should be easier now after the recent DataStore refactor.

Unless this behavior is wanted, in which case we might consider an env var to control it (XK6_BROWSER_CLEANUP=0?) and maybe a new Browser option.

Integration tests might be good to have as well, since the current DataStore unit tests missed this.

[inancgumus]

This seems buggy:

https://github.com/grafana/xk6-browser/blob/12eeeb59caa75cc2818172b096debf1aea153848/chromium/browser_type.go#L185-L191

It tries to clean up the directory after the BrowserType.Launch() returns.

[ankur22]

I Initially thought it was a race condition, maybe the mkdir needed to be "flushed" somehow before the removeDir was called. It turns out that the creation and deletion of the temporary directory works as expected, although it's incorrect as @inancgumus mentioned in the above post (cleanup probably should be run in its own goroutine and perform the cleanup when the context is closed).

Another process creates the directory if it doesn't exist which is when we run the command to execute the cmd to run chrome.

The fix is:

1. Create an integration test that fails
2. Create the fix by moving the cleanup into its own goroutine and waiting for the context to close.
3. Ensure that the integration test now passes

EDIT

1. It wasn't possible/easy to to make the cleanup of the temp dir using a goroutine. I think the lifecycle of the different processes might need a bit of refactoring so that it we can make the lifecycle of these processes more deterministic.
2. The fix was to instead create a cleanupFunc that is called from the browser.Close() func.