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: adapt the test.py script to build project tests in offline mode and accept arbitrary arguments for ctest #2682

Merged
merged 2 commits into from
Mar 24, 2025

Conversation

Comrade88
Copy link

@Comrade88 Comrade88 commented Mar 7, 2025

this pr suggests adding to test.py the ability to build "offline" mode tests. i.e. exclude tests that require access to global resources. such tests always fail in a restricted environment and create a false impression of the project's performance.
also added the ability to feed ctest arbitrary parameters to gain more control over running tests.

Galas' Sergey added 2 commits March 5, 2025 10:22
there are some tests that require access to global resources,
like google.com or seastar.io, which are not accessible from
the restricted environment. the tests give a false negative.
setting ENABLE_TESTS_ACCESSING_INTERNET to 0 maros allows us
to exclude such tests.
let's add a handle to test.py to pass the value for this macro
through cmake to build and run tests in standalone mode.
…ional args

sometimes you only need to run a subset of tests. test.py can
already enumerate the desired ones, but it happens that you need to
exclude a subset from the full list.
ctest -E takes a regular expression of test names to exclude. ctest
also provides more useful features that we may want to use in the future.
let's not repeat the parsing of the ctest command line, but instead
redirect all positional arguments of this script directly to ctest.
@@ -37,6 +37,8 @@
parser.add_argument('--smp', '-c', action="store",default='2',type=int,help="Number of threads for multi-core tests")
parser.add_argument('--verbose', '-v', action = 'store_true', default = False,
help = 'Verbose reporting')
parser.add_argument('--offline', action="store_true", default = False,
Copy link
Contributor

Choose a reason for hiding this comment

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

this change still makes sense even with #2683 .

@@ -36,6 +36,8 @@ using namespace seastar::net;

static const sstring seastar_name = "seastar.io";

#if SEASTAR_TESTING_WITH_NETWORKING
Copy link
Contributor

Choose a reason for hiding this comment

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

@Comrade88 , hi Galas, thank you for this fix! You've correctly identified that several tests need proper handling with the SEASTAR_TESTING_WITH_NETWORKING macro.

While your approach works, I've taken the initiative to implement an alternative solution in PR #2683 that centralizes all the network-dependency flags in a single location. This should improve code readability and maintenance by avoiding scattered conditional checks throughout the codebase.

Could you please take a look at the alternative implementation when you have a chance. I believe it addresses the same issue you've identified while providing a more consolidated approach to managing network-dependent tests.

test.py Outdated
@@ -26,6 +27,7 @@
parser = argparse.ArgumentParser(description="Seastar test runner")
parser.add_argument('--fast', action="store_true", help="Run only fast tests")
parser.add_argument('--name', action="store", help="Run only test whose name contains given string")
parser.add_argument('--skip', action="append", help="Skip tests whose name contains given string")
Copy link
Contributor

@tchaikov tchaikov Mar 8, 2025

Choose a reason for hiding this comment

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

how about just exposing a flag allowing user to pass random parameters to customize the behavior of ctest? like

./test.py --mode debug -- --test-timeout 3600

Copy link
Author

Choose a reason for hiding this comment

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

good point. done.
also i've dropped the dns test patch in favor of PR #2683

@Comrade88
Copy link
Author

is there anything else that needs to be changed here?

@xemul xemul requested a review from tchaikov March 17, 2025 12:05
@xemul
Copy link
Contributor

xemul commented Mar 17, 2025

@Comrade88 , the provided PR description is too shallow, please write more profound one

@Comrade88 Comrade88 changed the title conditional test exclusion tests: adapt the test.py script to build project tests in offline mode and accept arbitrary arguments for ctest Mar 17, 2025
@Comrade88
Copy link
Author

@xemul , done

@tchaikov
Copy link
Contributor

@Comrade88 currently the description of this PR puts:

conditional and in offline mode

i guess Pavel suggested to make it more specific.

@Comrade88
Copy link
Author

Comrade88 commented Mar 20, 2025

@tchaikov , sorry, I'm a bit absent-minded and didn't realize @xemul suggested changing the description, not the title. now it is clear that the total description is many times greater than the size of the changes, which are quite literal

@xemul xemul closed this in 89a89a5 Mar 24, 2025
@xemul xemul merged commit 89a89a5 into scylladb:master Mar 24, 2025
16 checks passed
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.

3 participants