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

test: remove hard-coded sequelize dialect #566

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

LoneRifle
Copy link
Contributor

Problem

While GoGovSG uses a variety of postgres-specific queries and features,
the codebase still mostly talks to the database via Sequelize. To facilitate
integration testing, we will have to use something other than postgres,
eg an in-memory embedded database like SQLite.

Solution

Remove the hard-coded dialect in Sequelize options and let the dialect
be driven from the database URI. This allows us to easily make use of
other database implementations where needed.

Considerations

If the hard-coded dialect is kept, integration tests would be limited to
running on CI or an elaborate development environment. Integration tests
could run on a developer environment by mocking the postgres client, but
this is tedious and does not account for potential mistakes in SQL query
syntax or behaviour

While GoGovSG uses a variety of postgres-specific queries and features,
the codebase still talks to the database via Sequelize. To facilitate
integration testing, we will have to use something other than postgres,
eg an in-memory embedded database like SQLite.

Remove the hard-coded dialect in Sequelize options and let the dialect
be driven from the database URI. This allows us to easily make use of
other database implementations where needed.
Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

This looks like a pretty fundamental change. What is the impact to system behavior and underlying semantics if the dialect is removed?

@LoneRifle
Copy link
Contributor Author

Per description - no impact to system behaviour, though dialect is now determined by the database URI, which prefixes the protocol (eg, mysql, postgres, sqlite etc)

@liangyuanruo liangyuanruo merged commit cc1502f into develop Sep 10, 2020
@liangyuanruo liangyuanruo deleted the test/no-sequelize-dialect branch September 10, 2020 09:49
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.

2 participants