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/ssx/single_sharded: fix -c1 and correct test file name #20152

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

bashtanov
Copy link
Contributor

Unsigned overflow prevented this test from running with -c1.
Test file name did not have _test suffix.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.1.x
  • v23.3.x
  • v23.2.x

Release Notes

  • none

@bashtanov
Copy link
Contributor Author

/dt

@bashtanov bashtanov requested a review from dotnwat June 26, 2024 19:39
@bashtanov
Copy link
Contributor Author

build failure unrelated

@bashtanov
Copy link
Contributor Author

/dt

@bashtanov bashtanov force-pushed the ssx-single-sharded-test-fixup branch from 3c50f99 to 4e8b69d Compare June 27, 2024 11:43
@bashtanov
Copy link
Contributor Author

test failure unrelated
#14139

@@ -80,7 +80,7 @@ struct caller {

SEASTAR_THREAD_TEST_CASE(single_sharded) {
ss::shard_id the_shard = ss::smp::count - 1;
ss::shard_id wrong_shard = std::max(ss::shard_id{0}, the_shard - 1);
ss::shard_id wrong_shard = ss::smp::count == 1 ? 0 : the_shard - 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

but if we run with -c1, there is no wrong shard, right? So I guess it would be more obvious if the part of the test requiring several shards would be clearly encapsulated in a code block (or maybe even a separate test) and wrong_shard would be declared there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you think of an existing test that requires multiple shards? What does it do if there is only one shard?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think its ok to assume a minimum shard count in tests, just have an assert in the test, something like

BOOST_REQUIRE_GT(ss::smp::count, 1)

There are tests in the tree that does this, better to assert out than a segfault.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants