Add tests - #4
Conversation
046932e to
2abc152
Compare
2abc152 to
2342ab8
Compare
EmmaLRussell
left a comment
EmmaLRussell
left a comment
There was a problem hiding this comment.
LGTM, just a couple of tiny queries to take or leave.
How were the test data files created? Could you include instructions in the README in case these need to be changed?
UPDATE: I just found the generate-test-fixtures script! So maybe mention that in the README..
Could add coverage?
| - uses: actions/checkout@v5 | ||
| - uses: actions/setup-node@v5 |
There was a problem hiding this comment.
No big deal, but there are newer versions of these actions.
| bounds: { | ||
| min: { lng: -70.0635, lat: 12.4124 }, | ||
| max: { lng: -69.8654, lat: 12.624 }, | ||
| }, |
There was a problem hiding this comment.
I guess you haven't had any floating point comparison issues with this test yet - might be an issue later with real bounds calculation..?
There was a problem hiding this comment.
Added expect.closeTo
| .get('/metadata') | ||
| .query({ model_release: '../private' }); | ||
|
|
||
| expect(response.status).toBe(400); |
There was a problem hiding this comment.
I wonder if this should be a 404 rather than 400? Request is well formed , but the data doesn't exist.
There was a problem hiding this comment.
Changed message to 'Unknown' instead of 'Invalid', and 400 to 404
| }); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| // Expect all admin1 values to be unique |
There was a problem hiding this comment.
Is this just a misleading comment? Looks like it's checking admin0 not admin1?
| // Expect all admin1 values to be unique | ||
| const uniqueAdmin0 = new Set(response.body.admin0); | ||
| expect(uniqueAdmin0.size).toBe(2); | ||
| expect(response.body.admin0).toEqual(['ETH', 'MLI']); |
There was a problem hiding this comment.
Not really sure you need to do the unique check if you're then going to test the exact values here anyway..?
| expect(uniqueAdmin0.size).toBe(2); | ||
| expect(response.body.admin0).toEqual(['ETH', 'MLI']); | ||
| expect(response.body.median).toHaveLength(2); | ||
| expect(response.body.median).toEqual(expect.arrayContaining([ |
There was a problem hiding this comment.
Is there any reason not to just use toStrictEqual here? Wouldn't need length check in that case i think.? Same goes for similar tests elsewhere.
|
|
||
| expect(response.status).toBe(400); | ||
| expect(response.body.error).toBe( | ||
| 'Missing required query parameters: model_release, properties, gene, mutation', |
There was a problem hiding this comment.
is this idea with requiring all these parameters to avoid sending huge deluge of data to lazy clients? Definitely think it makes sense to require model_release, I think an empty properties could potentially imply "just give me all properties". Maybe gene is required by all mutations could be implied?
Probably not important if we turn out to be our only client!
There was a problem hiding this comment.
Yeah, my original design did have 'empty list of properties implies give all properties', but the size of data made me want to force consumers to explicitly confirm that's what they want!
| @@ -0,0 +1,226 @@ | |||
| // WHERE I GOT UP TO - reviewing changes/tests as far down as this file | |||
7dad0a4 to
f578c57
Compare
Updated README
Added coverage |
2 participants
Unit and integration tests. The majority of the apparent 5000-line diff is from fixtures and package-lock.json.
To avoid making tests dependent on potentially changing data, the integration tests use small fixtures (relatively small compared to real data). These are generated by a script, which is configured by
tests/fixtures/fixture-config.json.Calls to DuckDB are mocked in unit tests.