loculus-project / loculus

An open-source software package to power microbial genomic databases
https://loculus.org
GNU Affero General Public License v3.0
34 stars 1 forks source link

Rename E2E tests "website integration tests" - because they mock a lot of things #2719

Open corneliusroemer opened 1 week ago

corneliusroemer commented 1 week ago

The fact we call the playwright website tests E2E is misleading: so much is mocked that it's really just an integration test to check.

corneliusroemer commented 1 week ago

IIRC, the reason @fengelniederhammer and Tobias set these tests up really on is that there's no good Astro testing framework. So these were always intended to be mostly website tests. Not covering the whole system (at least not the whole system as it is now).

Maybe the name E2E was sort of ok at the start (even then though the mocking means it's not E2E). As soon as loculus comprised ingest, preprocessing, silo preprocessing, they should have been renamed to something more appropriate.

Calling them E2E has the implications that people misunderstand what is actually tested by them (and what isn't), e.g. Anya thought they tested at least prepro and silo prepro, and didn't mock so much

chaoran-chen commented 1 week ago

I think that we need to rework the tests majorly and that there are a few considerations:

  1. In which direction do we want the tests to evolve? Towards only website integration tests or end-to-end tests?
  2. How much of the existing tests are we going to keep using anyways? (If we plan to delete and redo them anyways very soon, we might not need to spend the time to rename them.)
  3. How do we set the boundaries of the Loculus (core) software? Does it include the Nextclade pipeline or is the pipeline itself in some ways an user (here: user=the admin setting up a new instance) input?
    • Maybe we decide for something in-between: The Nextclade pipeline is an optional component, so some tests include it; whereas other tests mock the pipeline (to represent other potential pipelines).
fengelniederhammer commented 1 week ago

So these were always intended to be mostly website tests.

That's not entirely true. As we wrote the tests at the time, it served two purposes:

Also at the time, there was no real preprocessing pipeline, so we had to mock one.

I agree that it might be good to separate the two purposes. AFAIK playwright can also mock backends, which could be used for testing the website. But also for end-to-end tests, we could still use playwright.