grafana / xk6-browser

k6 extension that adds support for browser automation and end-to-end web testing via the Chrome Devtools Protocol
https://grafana.com/docs/k6/latest/javascript-api/k6-experimental/browser/
GNU Affero General Public License v3.0
337 stars 42 forks source link

Fix the deletion of the temporary directory so that it is deterministic in all cases #403

Open ankur22 opened 2 years ago

ankur22 commented 2 years ago

This issue is related to issue #288 and was identified in PR #323.

Blocked by grafana/k6#2432

During the implementation of #323, it became clear that there wasn't a deterministic way of cleaning up the temporary directory if the test didn't complete as expected i.e. there's no guarantee that during the unhappy path of a test running that the cleanup would be called. This is because there is no way for k6 to wait for all the cleanup actions to be performed before shutting itself down.

There is an issue open in grafana/k6#2432, and once it's complete it will enable us to cleanup the temporary directory in all cases (apart from if k6 just crashes etc).

This is reproducible by adding and running the following test to tests/browser_test.go:

func TestTmpDirCleanupOnContextClose(t *testing.T) {
    tmpDirPath := "./"

    ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(time.Duration(time.Second*5)))

    err := os.Setenv("TMPDIR", tmpDirPath)
    assert.NoError(t, err)
    defer func() {
        err = os.Unsetenv("TMPDIR")
        assert.NoError(t, err)
    }()

    b := newTestBrowser(t, withSkipClose(), withContext(ctx))
    p := b.NewPage(nil)
    p.Close(nil)

    matches, err := filepath.Glob(tmpDirPath + "xk6-browser-data-*")
    assert.NoError(t, err)
    assert.NotEmpty(t, matches, "a dir should exist that matches the pattern `xk6-browser-data-*`")

    cancel()

    // TODO: There needs to be a way to wait for the cleanup to occur before moving onto
    //       the next part of the test. This is what grafana/k6#2432 should implement.

    matches, err = filepath.Glob(tmpDirPath + "xk6-browser-data-*")
    assert.NoError(t, err)
    assert.Empty(t, matches, "a dir shouldn't exist which matches the pattern `xk6-browser-data-*`")
}
sniku commented 2 years ago

Just pitching in to the issue. With the latest main eb13672 I get about 1-2 directories left behind when running a 10VU script for 1min.

Error line similar to this appears in these cases: ERRO[0059] unlinkat /tmp/xk6-browser-data-3665988004: directory not empty category="Browser:Close" elapsed="0 ms" goroutine=83

Script and log attached.

ankur22 commented 2 years ago

Just pitching in to the issue. With the latest main eb13672 I get about 1-2 directories left behind when running a 10VU script for 1min.

Error line similar to this appears in these cases: ERRO[0059] unlinkat /tmp/xk6-browser-data-3665988004: directory not empty category="Browser:Close" elapsed="0 ms" goroutine=83

Script and log attached.

So this could be due to another goroutine still writing to this directory: https://github.com/golang/go/issues/23452.

EDIT: I confirmed this was the case when running the test on Windows.