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

cdrs-tokio test suite integration #741

Merged
merged 12 commits into from
Sep 15, 2022

Conversation

conorbros
Copy link
Member

No description provided.

@conorbros conorbros marked this pull request as ready for review August 11, 2022 23:35
@conorbros conorbros marked this pull request as draft August 11, 2022 23:36
@conorbros conorbros marked this pull request as ready for review August 17, 2022 03:46
@conorbros
Copy link
Member Author

Not ready for review yet, just trying to get the CI to run so I can check it's working.

@benbromhead
Copy link
Member

This seems stuck, are we pursuing this (or a todo later)?

@conorbros conorbros force-pushed the cdrs-tokio-integration branch 5 times, most recently from e4f7e6f to ad8f732 Compare August 26, 2022 02:25
shotover-proxy/Cargo.toml Outdated Show resolved Hide resolved
shotover-proxy/src/message/mod.rs Show resolved Hide resolved
shotover-proxy/Cargo.toml Show resolved Hide resolved
@conorbros
Copy link
Member Author

This PR enables us to run cdrs-tokio for our Cassandra integration tests. The cpp driver is hidden behind a feature that will also run if the feature is enabled. rstest is used to define what tests we want to run each driver for.

There is still some problems running cdrs-tokio on all the tests:

  • I will need to get a fix upstreamed to cdrs-tokio to re-enable the collections tests (Add set type functions krojew/cdrs-tokio#103).
  • Many of the tests don't work with cdrs-tokio, still need to investigate the reasons for all of these. They have been marked with TODOs

I need to re-enable the collections test before this can be merged.

After that, need to decide whether to:

  • Only merge once cdrs-tokio runs on all the tests.
  • Merge as is and follow up on problems in later PRs.

I'm leaning towards merging once cdrs-tokio passes on all the tests.

Getting all the setup code for adding new drivers in could be useful though, for example the scylla driver could also be added.

@rukai
Copy link
Member

rukai commented Aug 28, 2022

I'm leaning towards merging once cdrs-tokio passes on all the tests.

As long as we have integration test cases running on at least the cassandra_cpp driver and we are happy with the multi-driver abstraction then I am happy to merge.

@conorbros conorbros force-pushed the cdrs-tokio-integration branch 3 times, most recently from 9c64228 to 4681688 Compare August 30, 2022 11:39
@conorbros conorbros requested a review from rukai August 31, 2022 06:22
@rukai
Copy link
Member

rukai commented Sep 1, 2022

The test run time has halved from 40m -> 20m, I think a bunch of tests are no longer running.

@conorbros conorbros force-pushed the cdrs-tokio-integration branch 4 times, most recently from 7379af1 to 9de308c Compare September 7, 2022 05:01
@conorbros
Copy link
Member Author

Fixed the issue with it missing some tests, review can continue now.

Copy link
Member

@rukai rukai left a comment

Choose a reason for hiding this comment

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

I think we might end up removing the rstest approach later on so that we can reuse Cassandra clusters between tests that have identical Cassandra setups.

The rstest approach is fine for now though, let's land this and iterate further in main.

@conorbros conorbros linked an issue Sep 14, 2022 that may be closed by this pull request
@rukai rukai enabled auto-merge (squash) September 15, 2022 04:43
@rukai rukai merged commit a46fefa into shotover:main Sep 15, 2022
@conorbros conorbros deleted the cdrs-tokio-integration branch September 15, 2022 06:15
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.

Cassandra CPP Driver Test portability
3 participants