Skip to content

Add missing libgearman protocol tests - #524

Merged
esabol merged 2 commits into
gearman:masterfrom
esabol:add-missing-proctocol-tests
Oct 11, 2026
Merged

esabol merged 2 commits into
gearman:masterfrom
esabol:add-missing-proctocol-tests

Conversation

@esabol

@esabol esabol commented Oct 8, 2026 •

Copy link
Copy Markdown
Member

Following up on #521 (comment), this pull request adds two missing protocol tests for GEARMAN_COMMAND_GET_STATUS_UNIQUE and GEARMAN_COMMAND_STATUS_RES_UNIQUE.

@esabol
esabol requested a review from p-alik October 8, 2026 20:19
@p-alik

p-alik commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator

The values match the enum order in libgearman-1.0/protocol.h, so this looks correct to me. A few optional suggestions:

1. Assert the sentinel too

  ASSERT_EQ(41, int(GEARMAN_COMMAND_GET_STATUS_UNIQUE));
  ASSERT_EQ(42, int(GEARMAN_COMMAND_STATUS_RES_UNIQUE));
  ASSERT_EQ(43, int(GEARMAN_COMMAND_MAX));

Without this, someone can add a new command without adding a line here, which is how 41 and 42 went untested. With it, a new command makes the test fail until its assertion is added.

2. Cover the command table, not just the enum numbers

The test checks the enum values but not the entries in gearmand_command_info_list[] (libgearman/command.cc). If two rows were swapped, or STATUS_RES_UNIQUE had the wrong argc, the test would still pass. The mistake would only show at runtime. A loop would catch it:

for (int cmd= 0; cmd < int(GEARMAN_COMMAND_MAX); ++cmd)
{
  ASSERT_EQ(cmd, int(gearman_command_info(gearman_command_t(cmd))->code));
}
ASSERT_EQ(1, gearman_command_info(GEARMAN_COMMAND_GET_STATUS_UNIQUE)->argc);
ASSERT_EQ(6, gearman_command_info(GEARMAN_COMMAND_STATUS_RES_UNIQUE)->argc);

3. Out of scope, for a follow-up PR: gearman_strcommand() and gearman_enum_strcommand() in libgearman/command.cc use <= GEARMAN_COMMAND_STATUS_RES_UNIQUE as their upper bound. It should be < GEARMAN_COMMAND_MAX. As written, the next command added will hit assert(0) in debug builds and print __UNKNOWN in release builds.

4. Possible alternative: these opcode numbers are part of the wire protocol and must never change. static_assert(GEARMAN_COMMAND_X == N, ...) checks placed next to the enum would break the build on any reordering, so you wouldn't need to add a runtime assertion for each new command.

None of these should block merging.

@esabol
esabol force-pushed the add-missing-proctocol-tests branch from 1781528 to 866f539 Compare October 11, 2026 05:33
Verify that every entry in the command info table has the correct
code (matching its enum index) and the expected argc value. This
will catch altered rows that the previous enum-only assertions
would have missed.
@esabol
esabol force-pushed the add-missing-proctocol-tests branch from 866f539 to d660d23 Compare October 11, 2026 06:30
@esabol

esabol commented Oct 11, 2026 •

Copy link
Copy Markdown
Member Author

Suggestion 1 sounded like a good idea to me, so I added that easily enough.

Suggestion 2 was more difficult. It required the libgearman/command.h header, which is private, technically, and requires linking to libgearmancore.la, which this test file does not do. But I was already "in for a penny, in for a pound" by the time I figured that out. Therefore, I moved the for loop and argc tests (which I expanded to include the whole table) to the internals.cc test file, which already included the libgearman/command.h header and links to libgearmancore.la and therefore seemed like the more appropriate place for these tests.

I'm going to skip suggestions 3 and 4 at this time. Feel free to submit PRs for these if you want them.

@esabol
esabol requested a review from p-alik October 11, 2026 06:52

@p-alik p-alik 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 taking on both suggestions. Moving the table check to internals.cc makes sense given the private header and the libgearmancore link. I checked all the argc values against gearmand_command_info_list[] and they match.

One small note, not blocking: gearman_command_info() already asserts code == command, so in a debug build a swapped row would abort t/internals instead of failing the assertion cleanly. The swap is still caught, so I'm fine leaving it as is.

I'll send a separate PR for the gearman_strcommand() / gearman_enum_strcommand() upper bound.
The gearman_strcommand() / gearman_enum_strcommand() bound is already fixed on master in #525.

@esabol
esabol merged commit 03c55c9 into gearman:master Oct 11, 2026
22 checks passed
@esabol
esabol deleted the add-missing-proctocol-tests branch October 11, 2026 16:25
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