xk6-browser icon indicating copy to clipboard operation
xk6-browser copied to clipboard

Refactor test to use virtual filesystem

Open ankur22 opened this issue 2 years ago • 2 comments

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:

  • https://github.com/grafana/xk6-browser/pull/323#discussion_r897722737 -- this affirms that it was designed to work with tests
  • https://github.com/grafana/xk6-browser/pull/323#discussion_r897723801 -- use sync.Once

ankur22 avatar Jun 15 '22 12:06 ankur22

It occurred to me that this diff in #402 can also solve the concurrency problem without needing you to do anything more. 🤔🤞

inancgumus avatar Jun 15 '22 14:06 inancgumus

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.

ankur22 avatar Jun 15 '22 15:06 ankur22