nstlaurent / DoomLauncher

Doom Launcher is a doom launching utility and database for custom wads and pk3s
GNU General Public License v3.0
231 stars 20 forks source link

[bug] Tests are really slow #303

Open kenbot opened 4 months ago

kenbot commented 4 months ago

On my machine the tests take almost a minute, with only 100 odd tests. It sort of works now, but it doesn't scale. The solution to #302 wants granular isolated tests, which do a lot of clearing the database. This and deepening test coverage significantly slow it down.

The slowness is caused by repeatedly opening Sqlite connections on every request, each of which opens an OS file handle.

Options:

  1. Rig DataAccess to keep connections open longer. This is considered bad practice for networked RDBMSes like Postgres, but Sqlite people seem to encourage it a lot more because of the file-on-disk thing, especially if it's just a single desktop application with a single thread. OTOH: It's probably a big change, and it means something now has to have the responsibility for opening and closing transactions. It also risks exposing fascinating Sqlite behavioural quirks. I wasn't able to produce a speedup trying this out, although I might just be misunderstanding how it works.
  2. Introduce connection pooling Connection pooling works a bit differently for Sqlite than for traditional RDBMSes. I don't understand it well and I wasn't able to observe a speedup.
  3. In memory Sqlite (RECOMMENDED) This produces a massive ~30x (2 seconds for the whole suite!) speedup. Doing this needs a hopefully minor-ish refactor that will push all knowledge of the connection string to a single place at the "top of the world", which should be a solid code quality boost on its own. This means the conn str can be seamlessly swapped out. Disadvantage: it means the test behaviour is a little further away from real behaviour. I recommend that a small (<5) number of "smoke tests" still use the full on-disk Sqlite. Abundantly worth it imho.
  4. Use a Fake Object that implements IDbDataSourceAdapter (name?) backed by hashmaps. This would be very fast, but it's too far away from real behaviour for me. The Sqlite semantics are an important part of the behaviour. It would also require more code that itself might need tests.

If you're happy with 3, I'll probably fold it into it's own commit inside the PR for #302.

nstlaurent commented 4 months ago

I guess it depends on how you are running and your hardware. On my laptop using Visual Studio 2022 it takes 5 seconds total.

image

Edit: Even on appveyor they seem to be taking 8-10 seconds total: https://ci.appveyor.com/project/hobomaster22/doomlauncher/builds/49998394

kenbot commented 4 months ago

Ohhhhhh that's interesting. Maybe my laptop is just old (2018)? I'll just do #302 without touching this, and then figure out what's going on afterwards. Cheers. 👍