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

Setup test database for running tests for database #350

Merged
merged 6 commits into from
Mar 18, 2023

Conversation

cychu42
Copy link
Contributor

@cychu42 cychu42 commented Mar 15, 2023

Part of #238
This sets up a test database starchart_test in the container test_database, and it can be used to run tests without interfering with the other database container or any other database.

For convenience, I added a .env.test file, for devs to run cp .env.test .env to change the database connection string to the test database.

Changes

  • Add test database in in docker-compose.yml
  • Add .env.test
  • Ignore the volume at /mysql-data-test for git, Prettier, ESlint, and Docker.

Testing

  1. Run cp .env.test .env
  2. Run docker compose up -d to make containers like usual
  3. Run npm run steup to connect, push schema, and seed the database
  4. Run npm run db:studio to check that the database works

@cychu42 cychu42 added this to the Milestone 0.6 milestone Mar 15, 2023
@cychu42 cychu42 self-assigned this Mar 15, 2023
@cychu42
Copy link
Contributor Author

cychu42 commented Mar 15, 2023

I wonder if there's some issue with the what GitHub action is using to run hadolint. Saw the same test error in Won's PR that has nothing to do with Dockerfile.

@humphd
Copy link
Contributor

humphd commented Mar 15, 2023

I filed hadolint/hadolint-action#79 on the hadolint-action failures.

@cychu42 cychu42 marked this pull request as ready for review March 15, 2023 16:32
@cychu42
Copy link
Contributor Author

cychu42 commented Mar 15, 2023

Looks like it's fixed.

docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
* Change volume folder structure
* Adjust connection string for .env.example
@cychu42
Copy link
Contributor Author

cychu42 commented Mar 16, 2023

Glad you showed me that SQL setup file method. It's pretty interesting.

Both databases now run under the same database container.
I just use root user to access the databases for now, since I'm not sure why we need another account, given the containers are for local development and will be thrown away anyway.
/mysql/data will be where data is persisted, while /mysql/init has the SQL file for creating the two databases and the root user.

@cychu42 cychu42 requested a review from humphd March 16, 2023 21:40
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not rename mysql-data. We can move the .sql you created into config/ and then mount it where it needs to go in Docker.

For tests vs. running dev, the app uses dev-secrets/DATABASE_URL. How will we deal with differences here?

Ask @dadolhay if there is anything to know about with respect to using the root user like this. I think it's fine, but I'm not an expert.

Also, do you want to blow-away the test-db every time you init in your .sql? It might make it easier to run tests.

.env.example Outdated Show resolved Hide resolved
mysql/init/01-databases.sql Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
@cychu42
Copy link
Contributor Author

cychu42 commented Mar 17, 2023

For tests vs. running dev, the app uses dev-secrets/DATABASE_URL. How will we deal with differences here?

I'm not sure I understand. Is updating dev-secrets/DATABASE_URL not enough?

@humphd
Copy link
Contributor

humphd commented Mar 17, 2023

It is not. The DATABASE_URL in the env is only used for calling Prisma cli. At runtime, the app uses a secret.

@cychu42
Copy link
Contributor Author

cychu42 commented Mar 17, 2023

Looking at the test and dev scripts, we use SECRETS_OVERRIDE=1 to use what's inside /dev-secrets for testing and development, instead of using Docker secrets.

@cychu42
Copy link
Contributor Author

cychu42 commented Mar 17, 2023

How about this? I have applied changes according to your feedbacks, but I keep the original setup for starchart database intact in docker-compose.yml so we don't have to worry about changing much (including the secrets). I just add code relevant to the SQL file to make that test database.
If someone wants to switch to test database, they just run cp .env.test .env, and then run npm run setup or npx prisma db push if the schema isn't there already.

In my mind, I'm not sure it's necessary to drop the test database for testing, since tests should probably clean the test database before running tests, to avoid interference between each tests or any manual testing.

I haven't heard from Denes. Given both startchart user and root user have superuser access, I don't expect there to be a difference, and root user is now only used for the test database anyway.

@cychu42 cychu42 requested a review from humphd March 17, 2023 21:26
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this. The only thing I'd like to see improved is not having a duplicate .env in here. If we have a lot of these files, people have to remember to update them. Can we eliminate the need for that?

Obviously people running the tests (e.g., CI) would need to set this value to override it.

I don't want to make this impossible, but I also don't want more maintenance hassle.

@cychu42
Copy link
Contributor Author

cychu42 commented Mar 17, 2023

The .env.test is just an alternative to manually replacing the connection string.
I guess we can tell people to do that in CONTRIBUTION.md with copy/paste, unless we want to start setting the connection string env variable in every test script.

@cychu42
Copy link
Contributor Author

cychu42 commented Mar 17, 2023

If we don't want another .env fiIe, I guess adding it into scripts is cleaner than asking people to change connection string back and forth by hand every time? That would definitely need an instruction in CONTRIBUTION.md, so I added it.

@cychu42 cychu42 requested a review from humphd March 17, 2023 23:48
@@ -59,6 +59,8 @@ $ npm run db:studio

> **Note** `npm run build` needs to be executed the first time running the project. As it generates a `build/server.js` script that `npm run dev` depends on. Subsequent times, only `npm run dev` is needed to run the app in development mode.

> **Note** If you are running test scripts for the first time, change `DATABASE_URL` in `.env` to `DATABASE_URL="mysql://starchart:starchart_password@127.0.0.1:3306/starchart"` and run `npm run setup` to setup the test database. You can change this back afterward.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be done in this PR, but would it be worth to add a new setup:test script to do this instead?

Copy link
Contributor Author

@cychu42 cychu42 Mar 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps. If someone can write a script that has a command to parse the file to change the variable, runs npm run setup, then changes it back.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I think we can simply copy the setup command to setup:test, and override the value for DATABASE_URL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Eakam1007 do you want to file an issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I might just create a PR for this today. Seems simple enough (unless I am missing something)

@cychu42 cychu42 merged commit 95bbd3f into DevelopingSpace:main Mar 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants