RN-473: Add unit tests for data-lake-api #3882

rohan-bes · 2022-04-21T23:29:04Z

Issue RN-473:

Added a small suite of unit tests for @tupaia/data-lake-api. The tests are quite similar to the @tupaia/data-api tests, however they also require spinning up a test database for the data-lake. We just use the same postgres server as the regular tupaia database for this (in local as well as CI/CD).

Also ran into an issue with codeship builds failing due to OOM after adding the new test suite. Discussed here the plan is just to break the test suite into 2 batches.

Merge tasks

Update local data-lake-api .env vars

rohan-bes · 2022-04-21T23:36:40Z

codeship-steps.yml

-          command: './packages/devops/scripts/ci/testInternalDependencies.sh'
-        - name: Test web-frontend
-          command: './packages/devops/scripts/ci/testFrontend.sh web-frontend'
+        - type: parallel


Split the tests into 2 batches. Arguably I should've left the existing tests as batch 1 and just started a new batch, it might make it a bit more obvious where to extend the tests... but this is prettier 😝

Nice! Yeah this is prettier!

rohan-bes · 2022-04-21T23:39:44Z

packages/data-lake-api/.env.example

+DATA_LAKE_DB_USER=
+
+DB_PG_USER=
+DB_PG_PASSWORD=


Need the PG_USER to be able to create the data_lake_test database.

Are these test required only? If so can we add a comment like # only required for unit tests? Or rename these to UNIT_TEST_DB_PG_USER or something.

Yep test only. I'll keep the name as is as it's consistent with other packages, but I'll add a comment like you suggest 👍

rohan-bes · 2022-04-21T23:46:26Z

packages/data-lake-api/package.json

@@ -15,8 +15,11 @@
    "lint": "eslint . --ext .ts",
    "lint:all": "yarn run lint \"src/**/*.{ts,jsx}\"",
    "lint:fix": "yarn lint --fix",
-    "test": "jest --passWithNoTests",
-    "test:coverage": "yarn test --coverage"
+    "test": "yarn workspace @tupaia/data-lake-api check-test-data-lake-exists && DATA_LAKE_DB_NAME=data_lake_test jest --runInBand",


Like we do for the tests that require the tupaia database, run a check to see if the data-lake database exists

rohan-bes · 2022-04-21T23:51:31Z

packages/data-lake-api/package.json

-    "test:coverage": "yarn test --coverage"
+    "test": "yarn workspace @tupaia/data-lake-api check-test-data-lake-exists && DATA_LAKE_DB_NAME=data_lake_test jest --runInBand",
+    "test:coverage": "yarn test --coverage",
+    "update-test-data": "bash -c 'source .env && pg_dump -s -h data-lake-db.tupaia.org -U $DATA_LAKE_DB_USER -O $DATA_LAKE_DB_NAME -t analytics > src/__tests__/testData/testDataDump.sql'",


Explicitly pull the schema off the data-lake-db.tupaia.org, rather than what's configured for DATA_LAKE_DB_URL. This is because we set DATA_LAKE_DB_URL to localhost for the tests, and we need a real database here to pull the schema. Should be fine to use as we're using a read-only user and it's just one table 👍

Very handy!

biaoli0

Thanks @rohan-bes ! Always nice to have a set of unit tests to cover the code. I don't feel it will break anything for the devops changes, happy to give an approve. Just got a few comments and minor changes requested.

biaoli0 · 2022-05-01T23:36:43Z

packages/data-lake-api/.env.example

+DATA_LAKE_DB_USER=
+
+DB_PG_USER=
+DB_PG_PASSWORD=


Are these test required only? If so can we add a comment like # only required for unit tests? Or rename these to UNIT_TEST_DB_PG_USER or something.

biaoli0 · 2022-05-02T01:14:03Z

packages/data-lake-api/src/__tests__/fetchAnalytics.test.ts

+          organisationUnitCodes: ['NZ_AK', 'NZ_WG'],
+          dataElementCodes: ['BCD1TEST', 'BCD325TEST', 1],
+        },
+        /dataElementCodes*/,


Super cool to use regex here. Just one question, does this test case mean we it will throw an error contains dataElementCodes?

Saw comment // data element code is not a string in fetchEvents.test.ts in a similar test case, that helps to read. Suggest to add the same comment here.

Yep exactly, its sort of just to make the tests a bit more resilient to changes in the error message. But I'll take a look at these and improve either the messaging or the comments around the tests 👍

biaoli0 · 2022-05-02T02:06:19Z

packages/data-lake-api/scripts/checkTestDataLakeExists.sh

+echo -e "Error: $DATA_LAKE_DB_NAME database does not exist!\n\nTo create it, please run:\nyarn workspace @tupaia/data-lake-api setup-test-data-lake\n"
+exit 1


Suggested change

echo -e "Error: $DATA_LAKE_DB_NAME database does not exist!\n\nTo create it, please run:\nyarn workspace @tupaia/data-lake-api setup-test-data-lake\n"

exit 1

echo -e "Error: $DATA_LAKE_DB_NAME database does not exist!\n\nTo create it, please get .env secret from lastpass then run:\nyarn workspace @tupaia/data-lake-api setup-test-data-lake\n"

exit 1

^ A minor suggestion, that helps my first time setup in local.

Ah good call!

biaoli0 · 2022-05-02T02:14:31Z

packages/data-lake-api/src/__tests__/utilities/getTestDataLakeDatabase.ts

+export function getTestDatabase() {
+  if (!readDatabase) {
+    readDatabase = new DataLakeDatabase();


Suggested change

export function getTestDatabase() {

if (!readDatabase) {

readDatabase = new DataLakeDatabase();

export function getTestDatabase() {

if (!readDatabase) {

readDatabase = new DataLakeDatabase(getConnectionConfig());

Can we move getConnectionConfig() here? Just wanna make these functions be more consistent.

Sure thing 👍

biaoli0 · 2022-05-02T02:24:06Z

codeship-steps.yml

-          command: './packages/devops/scripts/ci/testInternalDependencies.sh'
-        - name: Test web-frontend
-          command: './packages/devops/scripts/ci/testFrontend.sh web-frontend'
+        - type: parallel


Nice! Yeah this is prettier!

biaoli0 · 2022-05-02T02:39:08Z

packages/devops/ci/testing.env.encrypted

Does it change automatically? Like yarn.lock changes after running yarn?

No, we actually have to manually update this using jet encrypt. Read here for more info

rohan-bes commented Apr 21, 2022

View reviewed changes

biaoli0 approved these changes May 2, 2022

View reviewed changes

rohan-bes added 3 commits May 2, 2022 16:07

RN-473: Add unit tests for data-lake-api

d7ccb9b

RN-473: Break tests into batches

0c1f04c

RN-473: PR Cleanups

63f6e74

rohan-bes force-pushed the rn-473-add-unit-tests-data-lake branch from 5c6c0da to 63f6e74 Compare May 2, 2022 06:07

rohan-bes merged commit 56b639f into dev May 2, 2022

rohan-bes deleted the rn-473-add-unit-tests-data-lake branch May 2, 2022 07:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RN-473: Add unit tests for data-lake-api #3882

RN-473: Add unit tests for data-lake-api #3882

rohan-bes commented Apr 21, 2022 •

edited

Loading

rohan-bes Apr 21, 2022

biaoli0 May 2, 2022

rohan-bes Apr 21, 2022

biaoli0 May 1, 2022

rohan-bes May 2, 2022

rohan-bes Apr 21, 2022

rohan-bes Apr 21, 2022

biaoli0 May 2, 2022

biaoli0 left a comment

biaoli0 May 1, 2022

biaoli0 May 2, 2022

rohan-bes May 2, 2022

biaoli0 May 2, 2022

rohan-bes May 2, 2022

biaoli0 May 2, 2022

rohan-bes May 2, 2022

biaoli0 May 2, 2022

biaoli0 May 2, 2022

rohan-bes May 2, 2022

		echo -e "Error: $DATA_LAKE_DB_NAME database does not exist!\n\nTo create it, please run:\nyarn workspace @tupaia/data-lake-api setup-test-data-lake\n"
		exit 1

RN-473: Add unit tests for data-lake-api #3882

RN-473: Add unit tests for data-lake-api #3882

Conversation

rohan-bes commented Apr 21, 2022 • edited Loading

Issue RN-473:

Merge tasks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biaoli0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohan-bes commented Apr 21, 2022 •

edited

Loading