IT: Enable more tests #325
Merged
Conversation
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
The test required just a minor adjustment to the log message, so that it matches the Rust Driver's message emitted upon repreparation.
wprzytula
added
bug
There was a problem hiding this comment.
Pull Request Overview
This PR enables additional integration tests and fixes a bug related to handling quoted (case‐sensitive) keyspace names, along with enhanced C++ exception messaging for better debug output.
- Enabled integration tests for prepared statement behavior, keyspace connection, and metric error handling.
- Fixed a bug rejecting quoted keyspace names in the Rust driver and improved C++ exceptions to include detailed error strings.
- Updated test filters in the Makefile to broaden MetricsTests coverage.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/src/integration/tests/test_prepared.cpp | Updated logger messages and added verification for expected logger behavior. |
| tests/src/integration/objects/session.hpp | Enhanced C++ exception message concatenation to aid debuggability. |
| scylla-rust-wrapper/src/session.rs | Improved handling of quoted keyspace names for case-sensitivity. |
| Makefile | Modified test filter patterns to enable broader MetricsTests. |
Comments suppressed due to low confidence (1)
Makefile:38
- Expanding the MetricsTests filter to use a wildcard may inadvertently enable tests that were previously excluded; please verify that the pattern only matches the intended tests.
:MetricsTests.*\
Comment on lines
288
to
+289
| throw Exception("Unable to Establish Session Connection: " + | ||
| session.connect_error_description(), | ||
| session.connect_error_description() + " - " + session.connect_error_message(), |
There was a problem hiding this comment.
The concatenation of error_description and error_message improves debuggability, but ensure that this format does not duplicate information if the error description already contains the detailed message.
Copilot uses AI. Check for mistakes.
Lorak-mmk
approved these changes
Jun 24, 2025
The test set up the logger criteria, yet it never checked the logger count. This was an obvious oversight that has now been corrected. As the sought string did not match the Rust driver's wording, I adjusted it. The test is not enabled for Cassandra. I tried to enable it, but it fails: it seems that the prepared ID stays the same even after the table is dropped and recreated. This is most probably due to an old Cassandra used in the CI.
When an exception is thrown in integration tests, the stringified error code would be included in the exception message, but not the error message. In order to make debugging easier, the error message is now included in the exception message as well. This exhibits the underlying Rust Driver's nested errors' messages.
The CPP Driver allows keyspace names to be case-sensitive if they are enclosed in quotes. CPP-Rust Driver, when encountered a quoted keyspace, would reject it with an error, because it verifies that a keyspace name only contains alphanumeric characters and underscores. Rust Driver does allow case-sensitive keyspace names, but this setting must be explicitly set by a boolean flag, and the name itself must not be quoted. This commit adds a check for quoted keyspace names, and if they are found, it removes the quotes and sets the case-sensitivity flag accordingly. This allows us to enable another integration test: `UseKeyspaceCaseSensitiveTests.ConnectWithKeyspace`.
The test uses cass_cluster_set_core_connections_per_host(), which was not yet implemented when the metrics were implemented. Now it's available, so we can enable the test.
As we now run more tests from the MetricsTests suite than not, it makes sense to negate the qualification of the tests, so that we say that we run the whole suite _except_ two tests. This way of specifying disabled tests explicitly makes them more discoverable for further development, so that we can easier see what's not yet supported and/or tested.
wprzytula
force-pushed
the
enable-more-tests
branch
from
ccc775e to
33f6baf
Compare
Lorak-mmk
approved these changes
Jun 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 PR enables some more integration tests and does some work needed for that.
The enabled tests are:
PreparedIDUnchangedDuringReprepare(along with a tinyFailFastWhenPreparedIDChangesDuringReprepareenhancement);UseKeyspaceCaseSensitiveTests.ConnectWithKeyspace;MetricsTests.ErrorsConnectionTimeouts.One bug was fixed
When a quoted (case-sensitive) keyspace name is passed, the underlying Rust Driver would reject it. Now it handles it properly.
Bonus
For better debuggability, C++ exceptions in IT now include the error message in their string, which exhibits Rust Driver's underlying errors.
Makes progress towards: #132
Pre-review checklist
[ ] I have implemented Rust unit tests for the features/changes introduced.Makefilein{SCYLLA,CASSANDRA}_(NO_VALGRIND_)TEST_FILTER.[ ] I added appropriateFixes:annotations to PR description.