Reorganise integration tests - part 2#1351
Merged
wprzytula merged 42 commits intoscylladb:mainfrom May 26, 2025
Merged
Conversation
wprzytula
added
the
area/testing
|
|
This was referenced May 10, 2025
Lorak-mmk
requested changes
May 10, 2025
Collaborator
Lorak-mmk
left a comment
Lorak-mmk
left a comment
There was a problem hiding this comment.
What I wrote about prepared I think also applies to batch and unprepared - they also could be a files instead of folders.
scylla/tests/integration/statements/prepared/prepared_inherits_config.rs
Outdated
Show resolved
Hide resolved
muzarski
suggested changes
May 13, 2025
scylla/tests/integration/statements/prepared/prepared_inherits_config.rs
Outdated
Show resolved
Hide resolved
wprzytula
force-pushed
the
reorganise-integration-tests
branch
from
ae1b69b to
f403aa1
Compare
Collaborator
Author
|
Rebased on main. |
As noted during the review of this PR, it's better to have all batch-related tests in a single file for better organization and maintainability.
`test_shard_out_of_range()` was put in `load_balancing.rs`, while `shards.rs` seems to be a much better place for it.
The only test that is left there regards latency latency_awareness, so such rename makes sense. And module_inception is no longer there.
Vector type is definitely more about a CQL collection than about the Session.
…ns.rs The test is definitely more about CQL collections than about the Session.
This test is about load balancing more than about the Session.
The `metadata_custom_timeouts.rs` file is renamed to `configuration.rs`, which is going to contain all the metadata fetching configuration tests.
This obviously belongs to `metadata/configuration.rs` category.
These are tests that simply check that driver keeps metadata of some kind. As these tests are similar in their concept, I believe it makes sense to have them all in one file.
Materialized views test suits metadata/contents.rs as well.
It belongs to `metadata/configuration.rs`.
Having read the contents of the test, I was confused what it actually tests. `git blame` shed some light, so I pasted the commit message as a doc comment: "This test case indicates that we support enough CQL types to parse schema keyspace information.". A-ha! But we no longer type check nor deserialize rows eagerly! The deserialization refactor seems to have rendered this test case useless. To fix this, I materialise all rows as Row, which actually performs the type check and deserialization. This way I virtually bring back the old, intended semantics of this test.
Tests `tests_timestamp()` and `test_timestamp_generator()` are extracted from `session/session.rs` into `statement/timestamps.rs`
There is a bunch of tests which check that statements are reprepared if the DB responds with DbError::Unprepared. They are extracted from `session/session.rs` into `statements/transparent_reprepare.rs`.
Test `test_unprepared_statement()` is extracted into `statements/unprepared.rs`.
Test `test_prepared_statement()` is extracted into `statements/prepared.rs`.
There was a test case in test_prepared_statement() that actually had nothing to do with prepared statements, but was testing the ser/de macros. It was extracted to a separate file.
Test `test_batch()` is extracted into `statements/batch.rs`.
The test is extracted into `statements/batch.rs`.
The test is moved into `statements/batch.rs`.
The test is extracted into `statements/prepared.rs`.
The test is about creating a new Session, so it fits there well.
Tests: - test_iter_works_when_retry_policy_returns_ignore_write_error() - test_iter_methods_with_modification_statements() are extracted into `session/pager.rs`.
The test is extracted into `statements/prepared.rs`.
The test is extracted into `statements/prepared.rs`.
The test is extracted to `statements/request_timeout.rs`.
The test case brings no value to our suite. There are plenty of other tests that call `Session::await_schema_agreement()`.
The test is extracted to `statements/named_bind_markers.rs`.
The remaining two tests in session.rs: - test_db_errors(), - test_rate_limit_exceeded_exception(), regard DB errors, so the file is renamed accordingly. This finishes dissolution of the old overloaded `session_test.rs` module. Hooray!
The test is extracted into `statements/prepared.rs`.
The only tests remaining in `statement.rs` are about coordinator enforcing, so the file is renamed accordingly.
The test is strictly about unprepared statements, so it's moved to the corresponding module.
The test is strictly about prepared statements, so it's moved to the corresponding module.
wprzytula
force-pushed
the
reorganise-integration-tests
branch
from
943f892 to
7b4dadb
Compare
Collaborator
Author
|
v1.2: addressed |
muzarski
approved these changes
May 26, 2025
Lorak-mmk
approved these changes
May 26, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a follow-up of #1350. Motivation is the same.
This PR, in contrast with the previous one, dissolves many large test files and splits them into more granular, tree-structured modules.
Sanity check
I deleted one test, which I believe to be redundant and pointless (
test_await_schema_agreement()). With this in mind, the number of tests before and after is correct, which means I didn't lose any test.Before:
After:
Pre-review checklist
[] I added relevant tests for new features and bug fixes.[ ] I have provided docstrings for the public items that I want to introduce.[ ] I have adjusted the documentation in./docs/source/.[ ] I added appropriateFixes:annotations to PR description.