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

feat: support multiple connections (sessions) in a single test file #180

Merged
merged 10 commits into from
Jul 11, 2023

Conversation

BugenZhao
Copy link
Collaborator

@BugenZhao BugenZhao commented Jul 5, 2023

Signed-off-by: Bugen Zhao i@bugenzhao.com

DuckDB's sqllogictest implementation provides the ability for establishing multiple connections in an slt file and specify which to use for each query or statement, which is helpful for testing database transactions.

This PR implements a similar feature, except that we will use a line connection <name> instead of the DuckDB's syntax since it is already utilized by our error matching feature.

This is a breaking change to the interfaces of the library crate (like Runner::new), so maybe we need to bump the minor version under 0.

@BugenZhao
Copy link
Collaborator Author

@wangrunji0408 @xxchan @skyzh Would you please help to review this? 🥰

Copy link
Member

@wangrunji0408 wangrunji0408 left a comment

Choose a reason for hiding this comment

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

Rest part LGTM!

sqllogictest-bin/Cargo.toml Outdated Show resolved Hide resolved
sqllogictest-engines/Cargo.toml Outdated Show resolved Hide resolved
sqllogictest/src/connection.rs Outdated Show resolved Hide resolved
Copy link
Member

@xxchan xxchan left a comment

Choose a reason for hiding this comment

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

Generally LGTM, nice work! Just some nits.

examples/connection/connection.slt Show resolved Hide resolved
sqllogictest/src/parser.rs Show resolved Hide resolved
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [0.15.0] - 2023-07-06

* Allow multiple connections to the database in a single test case, which is useful for testing the transaction behavior. This can be achieved by attaching a `connection foo` record before the query or statement.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add an example transaction test? Maybe use Postgres

Copy link
Member

Choose a reason for hiding this comment

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

Oh, but currently our CI doesn't have Postgres

Copy link
Member

Choose a reason for hiding this comment

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

But it's ok even if it's only tested locally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's an example that basically shows how it works.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I see it, but I mean a more “real” one 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. Let's add a comprehensive set of tests for the engines crate in the future.

sqllogictest/src/runner.rs Outdated Show resolved Hide resolved
sqllogictest/src/runner.rs Show resolved Hide resolved
sqllogictest/src/runner.rs Show resolved Hide resolved
sqllogictest/src/connection.rs Outdated Show resolved Hide resolved
Copy link
Member

@xxchan xxchan left a comment

Choose a reason for hiding this comment

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

Generally LGTM, nice work! Just some nits.

BugenZhao added 10 commits July 11, 2023 13:40
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@BugenZhao
Copy link
Collaborator Author

All comments resolved. Please take a look again. 🥰

@xxchan xxchan merged commit 9e8836e into risinglightdb:main Jul 11, 2023
@xxchan
Copy link
Member

xxchan commented Jul 11, 2023

new version published

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.

3 participants