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.

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

tests: enable/disable internet-dependent tests at runtime #2683

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tchaikov
Copy link
Contributor

@tchaikov tchaikov commented Mar 8, 2025

Previously, some tests in dns_test.cc and tls_test.cc that required internet connectivity were conditionally compiled using #if SEASTAR_TESTING_WITH_NETWORKING. However, several other internet-dependent tests were not properly disabled,
causing test failures in environments without internet access.

This change:

  • Replaces compile-time guards with runtime conditional execution using boost::unit_test::enable_if<SEASTAR_TESTING_WITH_NETWORKING> for tests requiring internet
  • Uses boost::unit_test::enable_if<!SEASTAR_TESTING_WITH_NETWORKING> for alternative tests that provide equivalent coverage without internet access

This approach improves maintainability, enables running the test suite in offline environments, and makes internet dependencies more explicit in the test code.

tchaikov added 2 commits March 8, 2025 08:34
…ng conflicts

Previously, we had two variants of test_simple_x509_client - one testing
with Google servers and another with a local server. These variants were
conditionally compiled using `#if SEASTAR_TESTING_WITH_NETWORKING` and
`#if !SEASTAR_TESTING_WITH_NETWORKING` macros respectively, ensuring only
be compiled based on networking availability.

In an upcoming change, we'll replace these compile-time conditionals with boost
decorators to dynamically enable/disable tests at runtime based on the same
`SEASTAR_TESTING_WITH_NETWORKING` macro. This will result in both test variants
being compiled simultaneously, causing naming conflicts.

This commit renames the test variants with distinct names to prepare for the
upcoming consolidation of network access testing.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Previously, some tests in dns_test.cc and tls_test.cc that required internet
connectivity were conditionally compiled using `#if SEASTAR_TESTING_WITH_NETWORKING`.
However, several other internet-dependent tests were not properly disabled,
causing test failures in environments without internet access.

This change:
- Replaces compile-time guards with runtime conditional execution using
  boost::unit_test::enable_if<SEASTAR_TESTING_WITH_NETWORKING> for tests
  requiring internet
- Uses boost::unit_test::enable_if<!SEASTAR_TESTING_WITH_NETWORKING> for
  alternative tests that provide equivalent coverage without internet access

This approach improves maintainability, enables running the test suite in
offline environments, and makes internet dependencies more explicit in the
test code.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
@tchaikov
Copy link
Contributor Author

tchaikov commented Mar 8, 2025

cc @Comrade88

@Comrade88
Copy link

now you reapeat these typedef's in every test file. may be it is better to put them somewhere central or even introduce special SEASTAR_TEST_CASE macro wrappers?

@tchaikov
Copy link
Contributor Author

tchaikov commented Mar 8, 2025

now you reapeat these typedef's in every test file. may be it is better to put them somewhere central or even introduce special SEASTAR_TEST_CASE macro wrappers?

so far, we have two test files including this decorator. once it repeats itself in more places, probably we should indeed have a central place for it. what do you think?

@Comrade88
Copy link

at the moment before my pr there was only one place in the project with a conditional disabling of online tests. I encountered a problem, found this place, understood what needed to be changed, and decided that this is how such issues are solved here.
later someone else will come, see the proposed typedefs and copy them to the next file, because this is how it is done here. in my opinion, it is better to immediately provide prospective users with a good example.

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.

2 participants