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.

Sign up for GitHub

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

ci: fix skipped tests #155

Merged
merged 11 commits into from
Sep 5, 2024
Merged

ci: fix skipped tests #155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0759fe1
types: convert column type duration to cass data type
muzarski Aug 5, 2024
6e0910c
ccm: get version from scylla
muzarski Aug 5, 2024
8e9f164
cass_version: don't hardcode scylla version
muzarski Aug 5, 2024
a1df254
test: check version only for cassandra
muzarski Aug 5, 2024
82804ad
ci: disable no_compact test
muzarski Aug 5, 2024
1ccd2bc
test: skip version check for scylla in nocompact test
muzarski Aug 5, 2024
c34a4d8
iterator: implement collection iterator for maps
muzarski Aug 5, 2024
7e7f24e
test_collection: adjust checking collection emptiness
muzarski Aug 5, 2024
aa7312f
test: revert changes from collection types test
muzarski Aug 5, 2024
8a15682
ci: enable duration tests for cassandra
muzarski Aug 5, 2024
d5cff5c
ci: bump scylla version to 6.0.0
muzarski Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ jobs:
working-directory: ./scylla-rust-wrapper
run: cargo test

- name: Run integration tests on Scylla 5.0.0
- name: Run integration tests on Scylla 6.0.0
env:
# Ignored tests are added in the end, after the "-" sign.
Tests: "ClusterTests.*\
Expand All @@ -51,8 +51,9 @@ jobs:
:CompressionTests.*\
:LoggingTests.*\
:-PreparedTests.Integration_Cassandra_PreparedIDUnchangedDuringReprepare\
:ExecutionProfileTest.InvalidName"
run: valgrind --error-exitcode=123 --leak-check=full --errors-for-leak-kinds=definite ./cassandra-integration-tests --scylla --version=release:5.0.0 --category=CASSANDRA --verbose=ccm --gtest_filter="$Tests"
:ExecutionProfileTest.InvalidName\
:*NoCompactEnabledConnection"
muzarski marked this conversation as resolved.
Show resolved Hide resolved
run: valgrind --error-exitcode=123 --leak-check=full --errors-for-leak-kinds=definite ./cassandra-integration-tests --scylla --version=release:6.0.0 --category=CASSANDRA --verbose=ccm --gtest_filter="$Tests"

- name: Upload test logs
uses: actions/upload-artifact@v3
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/cassandra.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ jobs:
:LoggingTests.*\
:-PreparedTests.Integration_Cassandra_PreparedIDUnchangedDuringReprepare\
:PreparedTests.Integration_Cassandra_FailFastWhenPreparedIDChangesDuringReprepare\
:*7.Integration_Cassandra_*\
:SslTests.Integration_Cassandra_ReconnectAfterClusterCrashAndRestart\
:ExecutionProfileTest.InvalidName"
:ExecutionProfileTest.InvalidName\
:*NoCompactEnabledConnection"
run: valgrind --error-exitcode=123 --leak-check=full --errors-for-leak-kinds=definite ./cassandra-integration-tests --version=4.0.7 --category=CASSANDRA --verbose=ccm --gtest_filter="$Tests"

- name: Upload test logs
Expand Down
2 changes: 1 addition & 1 deletion scylla-rust-wrapper/src/cass_types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ pub fn get_column_type(column_type: &ColumnType) -> CassDataType {
ColumnType::Text => CassDataType::Value(CassValueType::CASS_VALUE_TYPE_TEXT),
ColumnType::Timestamp => CassDataType::Value(CassValueType::CASS_VALUE_TYPE_TIMESTAMP),
ColumnType::Inet => CassDataType::Value(CassValueType::CASS_VALUE_TYPE_INET),
ColumnType::Duration => CassDataType::Value(CassValueType::CASS_VALUE_TYPE_DURATION),
ColumnType::List(boxed_type) => CassDataType::List {
typ: Some(Arc::new(get_column_type(boxed_type.as_ref()))),
frozen: false,
Expand Down Expand Up @@ -336,7 +337,6 @@ pub fn get_column_type(column_type: &ColumnType) -> CassDataType {
),
ColumnType::Uuid => CassDataType::Value(CassValueType::CASS_VALUE_TYPE_UUID),
ColumnType::Varint => CassDataType::Value(CassValueType::CASS_VALUE_TYPE_VARINT),
_ => CassDataType::Value(CassValueType::CASS_VALUE_TYPE_UNKNOWN),
}
}

Expand Down
16 changes: 10 additions & 6 deletions scylla-rust-wrapper/src/query_result.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ pub unsafe extern "C" fn cass_iterator_get_value(
) -> *const CassValue {
let iter = ptr_to_ref(iterator);

// Defined only for collections(list and set) or tuple iterator, for other types should return null
// Defined only for collections(list, set and map) or tuple iterator, for other types should return null
if let CassIterator::CassCollectionIterator(collection_iterator) = iter {
let iter_position = match collection_iterator.position {
Some(pos) => pos,
Expand All @@ -294,6 +294,11 @@ pub unsafe extern "C" fn cass_iterator_get_value(
Some(Value::CollectionValue(Collection::Tuple(tuple))) => {
tuple.get(iter_position).and_then(|x| x.as_ref())
}
Some(Value::CollectionValue(Collection::Map(map))) => {
let map_entry_index = iter_position / 2;
map.get(map_entry_index)
.map(|(key, value)| if iter_position % 2 == 0 { key } else { value })
}
_ => return std::ptr::null(),
};

Expand Down Expand Up @@ -625,13 +630,12 @@ pub unsafe extern "C" fn cass_iterator_from_collection(
return std::ptr::null_mut();
}

let map_iterator = cass_iterator_from_map(value);
if !map_iterator.is_null() {
return map_iterator;
}

let val = ptr_to_ref(value);
let item_count = cass_value_item_count(value);
let item_count = match cass_value_type(value) {
CassValueType::CASS_VALUE_TYPE_MAP => item_count * 2,
_ => item_count,
};

let iterator = CassCollectionIterator {
value: val,
Expand Down
15 changes: 14 additions & 1 deletion tests/src/integration/ccm/bridge.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,7 +849,20 @@ CassVersion CCM::Bridge::get_cassandra_version() {
// Get the version string from CCM
std::vector<std::string> active_cluster_version_command;
active_cluster_version_command.push_back(generate_node_name(1));

if (is_scylla_) {
// For scylla, we need to use `versionfrombuild`. `version` always returns 3.0.8.
active_cluster_version_command.push_back("versionfrombuild");

std::string ccm_output = execute_ccm_command(active_cluster_version_command);

// versionfrombuild returns a version directly
return CassVersion(ccm_output);
}

// Use `version` command for Cassandra.
active_cluster_version_command.push_back("version");

std::string ccm_output = execute_ccm_command(active_cluster_version_command);

// Ensure the version release information exists and return the version
Expand All @@ -861,7 +874,7 @@ CassVersion CCM::Bridge::get_cassandra_version() {

// Unable to determine version information from active cluster
throw BridgeException("Unable to determine version information from active Cassandra cluster \"" +
get_active_cluster() + "\"");
get_active_cluster() + "\"");
}

DseVersion CCM::Bridge::get_dse_version() {
Expand Down
2 changes: 1 addition & 1 deletion tests/src/integration/ccm/cass_version.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ class CassVersion {
std::string scylla_version_prefix = "release:";
std::string version(version_string);
if (version.compare(0, scylla_version_prefix.size(), scylla_version_prefix) == 0) {
version = "3.0.8";
version.replace(0, scylla_version_prefix.size(), "");
}
std::replace(version.begin(), version.end(), '.', ' ');
Comment on lines 281 to 286
Copy link
Collaborator

@wprzytula wprzytula Aug 12, 2024 •
edited by muzarski
Loading

Choose a reason for hiding this comment

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

How this new code actually retrieves the Scylla version? Or, if it's already retrieved, what variable stores it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or, if it's already retrieved, what variable stores it?

The version variable.

std::size_t found = version.find("-");
Expand Down
16 changes: 8 additions & 8 deletions tests/src/integration/integration.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,28 +83,28 @@
SKIP_TEST("Unsupported for Apache Cassandra Version " \
<< server_version_string << ": Server version " << version_string << "+ is required")

#define CHECK_VERSION(version) \
// Currently, we only check for the version if tests are being run against
// Cassandra cluster. It's because, some of the tests were unnecessarily
// skipped for Scylla. In the future (once it's needed) we might
// do some version restrictions for Scylla clusters as well.
#define SKIP_IF_CASSANDRA_VERSION_LT(version) \
do { \
CCM::CassVersion cass_version = this->server_version_; \
if (!Options::is_cassandra()) { \
cass_version = static_cast<CCM::DseVersion>(cass_version).get_cass_version(); \
} \
if (cass_version < #version) { \
if (!Options::is_scylla() && cass_version < #version) { \
SKIP_TEST_VERSION(cass_version.to_string(), #version) \
} \
} while (0)

#define CHECK_OPTIONS_VERSION(version) \
if (Options::server_version() < #version) { \
SKIP_TEST_VERSION(Options::server_version().to_string(), #version) \
}

#define CHECK_VALUE_TYPE_VERSION(type) \
#define CHECK_VALUE_TYPE_CASSANDRA_VERSION(type) \
CCM::CassVersion cass_version = this->server_version_; \
if (!Options::is_cassandra()) { \
cass_version = static_cast<CCM::DseVersion>(cass_version).get_cass_version(); \
} \
if (cass_version < type::supported_server_version()) { \
if (!Options::is_scylla() && cass_version < type::supported_server_version()) { \
SKIP_TEST_VERSION(cass_version.to_string(), type::supported_server_version()) \
}

Expand Down
14 changes: 8 additions & 6 deletions tests/src/integration/objects/collection.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,15 +194,17 @@ class Collection : public Object<CassCollection, cass_collection_free> {
} else {
FAIL() << "Invalid CassValueType: Value type is not a valid collection";
}

// Check if collection is empty (null).
// Scylla does not distinguish between an empty collection and NULL value.
// See: https://opensource.docs.scylladb.com/stable/cql/types.html#sets
size_t collection_size = cass_value_item_count(value);
if (collection_size > 0) {
is_null_ = false;
}

// Initialize the iterator
iterator_ = cass_iterator_from_collection(value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you drop this line then ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. iterator_ is a field. This code is called when constructor is called, and so this line initializes a field of a new object.


// Determine if the collection is empty (null)
const CassValue* check_value = cass_iterator_get_value(iterator_.get());
if (check_value) {
is_null_ = false;
}
}
}
};
Expand Down
6 changes: 3 additions & 3 deletions tests/src/integration/tests/test_basics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,8 @@ CASSANDRA_INTEGRATION_TEST_F(BasicsTests, BindBlobAsString) {
*/
CASSANDRA_INTEGRATION_TEST_F(BasicsTests, NoCompactEnabledConnection) {
CHECK_FAILURE;
CHECK_VERSION(3.0.16);
CHECK_VERSION(3.11.2);
SKIP_IF_CASSANDRA_VERSION_LT(3.0.16);
SKIP_IF_CASSANDRA_VERSION_LT(3.11.2);
CCM::CassVersion cass_version = server_version_;
if (!Options::is_cassandra()) {
if (server_version_ >= "6.0.0") {
Expand All @@ -343,7 +343,7 @@ CASSANDRA_INTEGRATION_TEST_F(BasicsTests, NoCompactEnabledConnection) {
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aren't we going to run into this if (!Options::is_cassandra()) { if (server_version_ >= "6.0.0") { branch when we update Scylla to 6.0?
If so, can you fix this in this PR?

Copy link
Collaborator Author

@muzarski muzarski Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No, Options::is_cassandra() actually returns true for Scylla as well. It's because Scylla identifies as Apache Cassandra server (such info is retrieved from ccm).

There are 3 possible CCM:ServerType enum values:

bool Options::is_cassandra() { return server_type_ == CCM::ServerType::CASSANDRA; }

bool Options::is_dse() { return server_type_ == CCM::ServerType::DSE; }

bool Options::is_ddac() { return server_type_ == CCM::ServerType::DDAC; }

I'm not sure about the difference between the types, but in Scylla's case, the server type is CASSANDRA. Options::is_scylla_ is a flag responsible for distinguishing between scylla and cassandra.

Copy link
Collaborator

Choose a reason for hiding this comment

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

wth is ddac?! Never heard about it before.

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://www.datastax.com/press-release/datastax-announces-datastax-distribution-apache-cassandratm

cass_version = static_cast<CCM::DseVersion>(cass_version).get_cass_version();
}
if (cass_version >= "4.0.0") {
if (!Options::is_scylla() && cass_version >= "4.0.0") {
SKIP_TEST("Unsupported for Apache Cassandra Version "
<< cass_version.to_string()
<< ": Server version must be less than v4.0.0 and either 3.0.16+"
Expand Down
4 changes: 2 additions & 2 deletions tests/src/integration/tests/test_by_name.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ CASSANDRA_INTEGRATION_TEST_F(ByNameTests, PreparedCaseSensitive) {
*/
CASSANDRA_INTEGRATION_TEST_F(ByNameTests, SimpleCaseSensitive) {
CHECK_FAILURE;
CHECK_VERSION(2.1.0);
SKIP_IF_CASSANDRA_VERSION_LT(2.1.0);

// Prepare, create, insert and validate
Statement statement(format_string(INSERT_CASE_SENSITIVE_FORMAT, table_name_.c_str()), 4);
Expand Down Expand Up @@ -391,7 +391,7 @@ CASSANDRA_INTEGRATION_TEST_F(ByNameTests, NullPrepared) {
*/
CASSANDRA_INTEGRATION_TEST_F(ByNameTests, NullSimple) {
CHECK_FAILURE;
CHECK_VERSION(2.1.0);
SKIP_IF_CASSANDRA_VERSION_LT(2.1.0);

// Prepare, create, insert and validate
Statement statement(format_string(INSERT_ALL_FORMAT, table_name_.c_str()), 7);
Expand Down
Loading
Loading