Skip to content

test: let Cassandra suites use a configurable contact point - #586

Open
Dodothereal wants to merge 3 commits into
ExtendDB:mainfrom
Dodothereal:issue-580-cassandra-test-contact-point
Open

Dodothereal wants to merge 3 commits into
ExtendDB:mainfrom
Dodothereal:issue-580-cassandra-test-contact-point

Conversation

@Dodothereal

@Dodothereal Dodothereal commented Oct 25, 2026 •

Copy link
Copy Markdown

What

Closes #580.

Reduced to documentation-only updates per maintainer request (#586 (comment)), as #681 introduced the EXTENDDB_TEST_CASSANDRA_CONTACT_POINT override logic in common/mod.rs on main:

  1. Updated devtools/run-cassandra-tests header prerequisites to document that the Rust direct tests read EXTENDDB_TEST_CASSANDRA_CONTACT_POINT (default 127.0.0.1:9042), which --contact-points does not configure, while preserving the safer loopback Docker example (-p 127.0.0.1:9042:9042).
  2. Updated --help line extraction in devtools/run-cassandra-tests (5,47p) to output the complete header prerequisites text.
  3. Updated module doc comments in crates/storage-cassandra/tests/direct_integration.rs and crates/storage-cassandra/tests/ttl_integration.rs to describe EXTENDDB_TEST_CASSANDRA_CONTACT_POINT (default 127.0.0.1:9042).

Why

Maintainer feedback noted that implementation was superseded by #681 on main, but the three documentation updates across the runner header and integration test modules are still needed on main.

Testing done

On head 9ed4fafebcf5345c3744a9b8b29b5a2706496244:

  • cargo fmt --check: passed.
  • devtools/run-cassandra-tests --help: verified complete documentation output through line 47.
  • Full git diff upstream/main..HEAD inspected: strictly limited to the three documentation files (runner header, direct integration header, ttl integration header).
  • Live Cassandra and full CI integration test suites were not rerun locally for this documentation-only update. GitHub Actions check runs have not yet executed on this commit.

Checklist

  • I have read CONTRIBUTING.md
  • Code is formatted
  • Documentation updated per maintainer review
  • No breaking changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache License 2.0 and I agree to the Developer Certificate of Origin.

This follow-up update was prepared using Gemini 3.8 Flash with automated local verification, without claiming human review.

Read EXTENDDB_TEST_CASSANDRA_CONTACT_POINT in both the shared test config and the skip probe, retaining 127.0.0.1:9042 by default and bracketing an unbracketed IPv6 host. Test the pure resolver so parallel tests never observe a changed environment, and mention the variable in the runner.

Signed-off-by: Dodothereal <129273127+Dodothereal@users.noreply.github.com>

@LeeroyHannigan LeeroyHannigan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for picking this up. I ran it against a cassandra:4.1 node on 127.0.0.1:19586 with the variable set, direct_integration passed 150 of 150 and ttl_integration 27 of 28, and nothing connected to 9042. The TTL failure is on main too, it's #566.

With the variable pointing at an unused port the suites skip and the message names that port, and with EXTENDDB_TEST_CASSANDRA=required they fail with the port in the error. fmt, clippy on 1.97.0 and the 1.88 build pass.

There are four things I'd like changed, and the first three have inline comments:

  1. The skip probe only tries the first resolved address, the driver tries all of them.
  2. The IPv6 bracketing isn't needed and rewrites some values.
  3. The runner header comment needs a couple of fixes.
  4. The module docs in direct_integration.rs (line 7) and ttl_integration.rs (line 6) still say the suites use 127.0.0.1:9042. Something like "at the contact point in EXTENDDB_TEST_CASSANDRA_CONTACT_POINT (default 127.0.0.1:9042)" would cover it. Line 6 of ttl_integration.rs also says there's no Cassandra CI workflow, which isn't true anymore. Fixing that is optional.

Once those are in I'll approve. #591 changes the same two functions under a different variable name, I'll move it onto yours after this merges. CI was waiting for a maintainer to approve the runs, I've approved them.

let reachable = contact_point
.to_socket_addrs()
.ok()
.and_then(|mut addrs| addrs.next())

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This only tries the first address the name resolves to, and the driver tries all of them. With a name that resolves to ::1 and then 127.0.0.1, and the node listening on 127.0.0.1 only, the probe skipped and the driver connected. So without required the tests returned early and passed against a live node, and since the harness captures a passing test's stderr, nothing said they'd skipped. That's what localhost:PORT would do on a host that returns ::1 first for localhost, against a node published with -p 127.0.0.1:PORT:9042. Trying every address fixes it:

let reachable = contact_point.to_socket_addrs().is_ok_and(|mut addrs| {
    addrs.any(|addr| {
        std::net::TcpStream::connect_timeout(&addr, std::time::Duration::from_secs(2)).is_ok()
    })
});

let Some((host, port)) = contact_point.rsplit_once(':') else {
return contact_point;
};
if host.contains(':') && !host.starts_with('[') {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think this branch is needed. to_socket_addrs already parses ::1:19586 as [::1]:19586, and with the branch removed the probe and the driver both reached a listener on [::1] through ::1:19588. It also rewrites other values with a second colon, ::1 becomes [:]:1 and 127.0.0.1:19586,127.0.0.1:19587 becomes [127.0.0.1:19586,127.0.0.1]:19587, and that's the text the skip message prints. I'd drop it and the ::1:19042 assertion, which leaves trim, the blank check and the default.

Comment thread devtools/run-cassandra-tests Outdated
# with PasswordAuthenticator enabled. The official cassandra:4.1 image does
# with PasswordAuthenticator enabled. The direct Rust test harness reads
# EXTENDDB_TEST_CASSANDRA_CONTACT_POINT when it is set; the --contact-points
# option below configures the extenddb server and Python suite. The official cassandra:4.1 image does

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

--contact-points is documented above, at line 15. This line is also 105 columns where the rest of the block wraps near 80, and the two new lines push the docker run command out of -h, which prints sed -n '5,35p' (line 72) and now stops at "so patch it after starting:". Something like this, with the range at 5,47p:

#     with PasswordAuthenticator enabled. The Rust direct tests read
#     EXTENDDB_TEST_CASSANDRA_CONTACT_POINT (default 127.0.0.1:9042), which
#     --contact-points above doesn't set. The official cassandra:4.1 image
#     does not honour CASSANDRA_AUTHENTICATOR, so patch it after starting:

This one's optional. Exporting EXTENDDB_TEST_CASSANDRA_CONTACT_POINT="$CONTACT_POINTS" in the script would point the Rust suites at the same node as the server, the way devtools/run-tests exports EXTENDDB_TEST_PG_CONNECTION_STRING.

use super::resolve_test_contact_point;

#[test]
fn contact_point_defaults_and_honours_an_override() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This one's optional. The test covers the string function, but nothing fails if test_config or the probe goes back to the literal address. Pulling out fn config_for(contact_point: String) and fn node_reachable(contact_point: &str) -> bool would let two short tests catch both, one that config_for("127.0.0.1:19042") keeps that contact point, and one that node_reachable is true for a bound TcpListener and false after it's dropped. It's about 20 lines.

Probe every resolved address, leave nonblank contact points unchanged, and
cover configuration forwarding and loopback reachability. Correct the
runner help range and integration-suite contact-point documentation.

Signed-off-by: Dodothereal <129273127+Dodothereal@users.noreply.github.com>

@LeeroyHannigan LeeroyHannigan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I re-ran the round-1 items against 0299470. In a container where localhost resolves to ::1 and then 127.0.0.1, with a listener on 127.0.0.1 only, node_reachable returns true and the previous first-address probe returns false, and probe_tries_later_resolved_addresses fails if the probe goes back to the first address.

resolve_test_contact_point returns ::1:19042, ::1 and 127.0.0.1:19586,127.0.0.1:19587 unchanged, -h now prints through the last prerequisite line, and the config_for and node_reachable tests fail when either goes back to the literal address.

Against cassandra:4.1 on 127.0.0.1:19586, test_put_and_get_item_pk_only passed with the variable set, including under EXTENDDB_TEST_CASSANDRA=required, and with the variable on a closed port it skipped and printed skipping: no Cassandra at 127.0.0.1:19587.

LeeroyHannigan
LeeroyHannigan previously approved these changes Oct 29, 2026

@LeeroyHannigan LeeroyHannigan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you for this @Dodothereal 🚀

@LeeroyHannigan

Copy link
Copy Markdown
Collaborator

Thank you for this. #681 added the same EXTENDDB_TEST_CASSANDRA_CONTACT_POINT override to crates/storage-cassandra/tests/common/mod.rs on main, so this PR now conflicts with it. Three of its comment changes are still needed on main: the header of devtools/run-cassandra-tests does not mention the variable, and the module comments of direct_integration.rs and ttl_integration.rs still say the tests need Cassandra at 127.0.0.1:9042. If you reduce the PR to those changes, we will review it; otherwise we will close it.

This branch has not been deployed

No deployments
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 direct test harness hardcodes 127.0.0.1:9042 with no override

2 participants