scylladb / cpp-rust-driver

API-compatible rewrite of https://github.com/scylladb/cpp-driver as a wrapper for Rust driver.
GNU Lesser General Public License v2.1
16 stars 11 forks source link

types: Implement API functions for `duration` type #135

Closed muzarski closed 3 months ago

muzarski commented 4 months ago

This PR implements following set of API functions related to duration type:

Pre-review checklist

muzarski commented 4 months ago

@Lorak-mmk @wprzytula Do you know how to enable the tests for the duration type? I see there is a tests/src/integration/values/duration.hpp file but I'm not sure how to enable it in CI.

muzarski commented 4 months ago

@Lorak-mmk @wprzytula Do you know how to enable the tests for the duration type? I see there is a tests/src/integration/values/duration.hpp file but I'm not sure how to enable it in CI.

I see that the duration tests were run in CI: 12 tests from CassandraTypes/CassandraTypesTests/7, where TypeParam = test::driver::NullableValue<test::driver::values::Duration>. I'm curious why Decimal tests are not being run (decimal is unimplemented type).

Lorak-mmk commented 4 months ago

@Lorak-mmk @wprzytula Do you know how to enable the tests for the duration type? I see there is a tests/src/integration/values/duration.hpp file but I'm not sure how to enable it in CI.

To be honest I'm not sure how those tests work. I can see that both duration and decimal have some special handling. Those tests are defined in tests/src/integration/tests/test_cassandra_types.cpp I can see that there is some specialization for Duration: https://github.com/scylladb/cpp-rust-driver/blob/805ef4c3a3f35e35d9e0736a86e47b2c600c3c4e/tests/src/integration/tests/test_cassandra_types.cpp#L98 And later some registration for types excluding duration: https://github.com/scylladb/cpp-rust-driver/blob/805ef4c3a3f35e35d9e0736a86e47b2c600c3c4e/tests/src/integration/tests/test_cassandra_types.cpp#L884-L891 There is also some instantiation that includes decimal: https://github.com/scylladb/cpp-rust-driver/blob/805ef4c3a3f35e35d9e0736a86e47b2c600c3c4e/tests/src/integration/tests/test_cassandra_types.cpp#L925-L930

Lorak-mmk commented 4 months ago

@Lorak-mmk @wprzytula Do you know how to enable the tests for the duration type? I see there is a tests/src/integration/values/duration.hpp file but I'm not sure how to enable it in CI.

I see that the duration tests were run in CI: 12 tests from CassandraTypes/CassandraTypesTests/7, where TypeParam = test::driver::NullableValue<test::driver::values::Duration>. I'm curious why Decimal tests are not being run (decimal is unimplemented type).

Where do you see that? I don't see any occurence of decimal or duration in CI logs.

muzarski commented 4 months ago

@Lorak-mmk @wprzytula Do you know how to enable the tests for the duration type? I see there is a tests/src/integration/values/duration.hpp file but I'm not sure how to enable it in CI.

I see that the duration tests were run in CI: 12 tests from CassandraTypes/CassandraTypesTests/7, where TypeParam = test::driver::NullableValue<test::driver::values::Duration>. I'm curious why Decimal tests are not being run (decimal is unimplemented type).

Where do you see that? I don't see any occurence of decimal or duration in CI logs.

build job, step Run integration tests on Scylla 5.0.0, line 577

Lorak-mmk commented 4 months ago

I see. Ctrl + f seems to be quite unreliable in those logs...

muzarski commented 4 months ago

v2: rebased on master

muzarski commented 3 months ago

v3:

muzarski commented 3 months ago

v3.1: rebased on master

Lorak-mmk commented 3 months ago

One more question: before you mentioned some tests that were incorrectly skipped. Is that fixed now?

muzarski commented 3 months ago

One more question: before you mentioned some tests that were incorrectly skipped. Is that fixed now?

Yes, there is an issue for that: https://github.com/scylladb/cpp-rust-driver/issues/144. Do you want me to fix it before merging this PR?

Lorak-mmk commented 3 months ago

It can be a separate PR - but I don't want to postpone it too long.