Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial development #1

Closed
4 of 5 tasks
chaoran-chen opened this issue Sep 2, 2024 · 10 comments
Closed
4 of 5 tasks

Initial development #1

chaoran-chen opened this issue Sep 2, 2024 · 10 comments
Assignees
Labels
enhancement New feature or request

Comments

@chaoran-chen
Copy link
Member

chaoran-chen commented Sep 2, 2024

Testset Folder

LAPIS-SILO-e2e
│   README.md
│   docker-compose.yml    
│   ...
│
└───testsets
    ├───testset1
    │   │   port.txt
    │   ├───data
    │   │       database_config.yml
    │   │       input_file.ndjson.zst
    │   │       reference_genomes.json
    │   │       ...
    │   │
    │   │
    │   └───tests
    │           queryAllAvailableData.query.json
    │           queryAllAvailableData.result.ndjson.zst
    │           onlyGetNullSequences.query.json
    │           onlyGetNullSequences.result.json
    ├───cchf_testset
    │       ...
    └───west_nile_zaire_testset
            ...
        ...
    
    

Test file

{
  name: string,
  request: {
    resource: same as fetch() API,
    options: same as fetch() API
  },
  bodyMatcher: "<one of the predefined matchers>",
  expectedResult: {
    statusCode: integer,
    body: {
      filePath: string, // or...
      raw: any // if the body is directly in the file
    },
    header: any // optional (exclusive with statusCode) and to be added in the future
  },
  expectedRuntime: integer // Optional and to be added in the future
}

Matchers

  • stringEquality - parses the result as a string and expects it to be exactly the same as the expectedResult
  • fastaOrderInsensitive
  • tsvOrderInsensitive

The matcher receives paths to two files. It returns { success: boolean, message?: string }.

Starting SILO & LAPIS containers

  • Get all test folders
  • Run a docker-compose file that starts all LAPIS and SILO containers
    • Ports of the LAPIS and SILO containers are assigned based on ____ / saved in ____
  • Execute test-runner with the LAPIS port and test folder

Test runner execution

./runner --testset testset1 --silo-url localhost:8080 --lapis-url localhost:8081
./runner --testset testset1 --silo-url localhost:8082 --lapis-url localhost:8083
./runner --testset testset1 --silo-url localhost:8084 --lapis-url localhost:8085

Tasks

(drafted together with @Taepper)

@corneliusroemer
Copy link
Contributor

Great! I realize reading this that a few things weren't clear based on my description. Your choices make a lot of sense!

Two comments:

  1. Let's please not use JSON to define tests (i.e. the test file). JSON forbids comments, and we want to comment tests sometimes. Also JSON is annoyingly verbose, let's use YAML for the test definition or .js?
  2. Will there be a diff if matcher fails? Might be great to have it as semantically as possible, i.e. when one field in metadata.tsv fails you get a report of exactly that etc. Could be done as followup though, initially there should be no failures

@fengelniederhammer
Copy link
Contributor

Test runner execution

./runner --testset testset1 --silo-url localhost:8080 --lapis-url localhost:8081
./runner --testset testset1 --silo-url localhost:8082 --lapis-url localhost:8083
./runner --testset testset1 --silo-url localhost:8084 --lapis-url localhost:8085

I would prefer to simply use npm run test, which executes vitest and not write a test runner ourselves. If someone wants to execute a specific subset of tests, we can still document how to do that with vitest.
Also I imagine a collection of constants in the code (or a config file) rather than passing the URLs:

const LAPIS_URLS = {
  cchfWithDateToSortBy: 'localhost:8080',
  cchfWithoutDateToSortBy: 'localhost:8081',
  ...
}

Why would the tests need SILO URLs? My understanding was that we want to test the LAPIS API?

The matcher receives paths to two files. It returns { success: boolean, message?: string }.

Usually matchers in the JS world work like expect(actualResult).toMatch(expectedResult). I think I would stick to that and try to extend the matchers that vitest provides. I'm quite sure that it's possible somehow.

@chaoran-chen
Copy link
Member Author

Yes, you're right - I agree on all points.

@Taepper
Copy link
Collaborator

Taepper commented Sep 3, 2024

Should vitest be executed from this repo or does only the data live here and the test-runner is in the LAPIS repo?

@chaoran-chen
Copy link
Member Author

Regarding JSONs, we could have a field like "comment" or "description" that can be filled with any string.

@fengelniederhammer
Copy link
Contributor

Should vitest be executed from this repo or does only the data live here and the test-runner is in the LAPIS repo?

IMO it would be better if this repo bundles everything and the other repos only need to execute a predefined command.

Regarding JSONs, we could have a field like "comment" or "description" that can be filled with any string.

I would vote for using Typescript files instead of JSON for defining the tests. The files can be as simple as returning an object, which is essentially a better version of JSON.

@corneliusroemer
Copy link
Contributor

I am strongly against JSON for anything but data in transit, it's a bad format for source code (which test definitions are).

Typescript sounds good to me.

Indeed, this repo should contain everything and package into as high level an API as possible. Though one can start lower level as it's easier during development. For a start, you want to be able to run npm run test as @fengelniederhammer suggested inside the container that contains the runner.

Eventually would be good to maybe just expose this as a Github action to be callable as a single step in LAPIS and SILO. But let's not start with perfect, simple is much better for now given the urgency.

@corneliusroemer corneliusroemer added the enhancement New feature or request label Sep 3, 2024
@chaoran-chen
Copy link
Member Author

The advantage of a JSON or another format for data would be that it would be easy to read from a different programming language. This can be useful if we want to develop an slow but simple reference implementation to check the results.

@fengelniederhammer
Copy link
Contributor

Initial development seems done.

@Taepper
Copy link
Collaborator

Taepper commented Sep 26, 2024

Initial development seems done.

.. and everything that is not done can be added as issues when desired :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants