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

Refactor test to use virtual filesystem #404

Open ankur22 opened 2 years ago

ankur22 commented 2 years ago

This issue was identified in grafana/xk6-browser#323.

The Storage type was designed to accept different filesystems, specifically for virtual filesystems to be able to test features that work with the FS.

Currently the test to check that the temporary directory is deleted writes and reads of the local FS. This is problematic since the test looks for directories which follow a certain pattern, and if the test were to fail then future tests would fail unless we manually delete the directories that were created. The use of a virtual FS would negate this so that subsequent test runs would not fail due to past failures.

It's also worth making Storage thread safe by using sync.Once.

I don't know the context behind why we're doing the way we're doing it.

I'm not sure about it myself. In general, it's a good idea to abstract away FS function calls so that you can mock them out in tests and avoid touching the FS (or use a virtual FS like afero), but from what I've seen we're not using them in our tests, or anywhere else.

@inancgumus originally wrote this, so WDYT? Should we remove fsMkdirTemp and fsRemoveAll and just use os directly?

_Originally posted by @imiric in https://github.com/grafana/xk6-browser/pull/323#discussion_r897719181_

Other comments:

inancgumus commented 2 years ago

It occurred to me that this diff in #402 can also solve the concurrency problem without needing you to do anything more. šŸ¤”šŸ¤ž

ankur22 commented 2 years ago

It occurred to me that this diff in #402 can also solve the concurrency problem without needing you to do anything more. šŸ¤”šŸ¤ž

Oh nice, I'm working on #402 first, so let's see.