Add RFC9669 conformance tests and update bpf_conformance submodule #980
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Update bpf_conformance submodule to include fixes for movsx, smod, ja32, and div/mod-by-zero tests - Add RFC9669 LLM conformance tests - Add missing bswap conformance tests - Document TEST_CONFORMANCE_FAIL macro usage Signed-off-by: Alan Jowett <alan.jowett@microsoft.com>
WalkthroughUpdate submodule reference for external/bpf_conformance and introduce TEST_CONFORMANCE_FAIL macro along with expanded conformance test suite covering additional byte-swap and RFC 9669 generated test cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/test/test_conformance.cpp`:
- Around line 31-35: The test currently hard-codes the plugin error code "1"
when checking the reason for TEST_RESULT_ERROR; update the assertion so it does
not depend on a magic "1". In the block that checks
bpf_conformance_test_result_t::TEST_RESULT_ERROR (using expected_result, reason,
expected_reason), either parameterize the expected exit code (e.g.,
expected_code) and build the expected message dynamically, or assert that reason
starts_with("Plugin returned error code ") and ends_with(expected_reason) /
contains the expected_reason suffix rather than matching the full string; adjust
the REQUIRE accordingly so the test accepts any numeric exit code while still
verifying the expected_reason.
Comment on lines
+31
to
+35
| if (expected_result == bpf_conformance_test_result_t::TEST_RESULT_ERROR) { | ||
| REQUIRE(reason == "Plugin returned error code 1 and output " + expected_reason); | ||
| } else { | ||
| REQUIRE(reason == expected_reason); | ||
| } |
There was a problem hiding this comment.
Avoid hard‑coding error code in error‑reason comparison.
If the conformance checker ever returns a non‑1 exit code (platform change or future update), ERROR tests will fail even when the reason matches. Match prefix/suffix instead of the exact code or parameterize the expected code.
Proposed change
- if (expected_result == bpf_conformance_test_result_t::TEST_RESULT_ERROR) {
- REQUIRE(reason == "Plugin returned error code 1 and output " + expected_reason);
- } else {
- REQUIRE(reason == expected_reason);
- }
+ if (expected_result == bpf_conformance_test_result_t::TEST_RESULT_ERROR) {
+ const std::string prefix = "Plugin returned error code ";
+ const std::string suffix = " and output " + expected_reason;
+ REQUIRE(reason.rfind(prefix, 0) == 0);
+ REQUIRE(reason.size() >= prefix.size() + suffix.size());
+ REQUIRE(reason.compare(reason.size() - suffix.size(), suffix.size(), suffix) == 0);
+ } else {
+ REQUIRE(reason == expected_reason);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (expected_result == bpf_conformance_test_result_t::TEST_RESULT_ERROR) { | |
| REQUIRE(reason == "Plugin returned error code 1 and output " + expected_reason); | |
| } else { | |
| REQUIRE(reason == expected_reason); | |
| } | |
| if (expected_result == bpf_conformance_test_result_t::TEST_RESULT_ERROR) { | |
| const std::string prefix = "Plugin returned error code "; | |
| const std::string suffix = " and output " + expected_reason; | |
| REQUIRE(reason.rfind(prefix, 0) == 0); | |
| REQUIRE(reason.size() >= prefix.size() + suffix.size()); | |
| REQUIRE(reason.compare(reason.size() - suffix.size(), suffix.size(), suffix) == 0); | |
| } else { | |
| REQUIRE(reason == expected_reason); | |
| } |
🤖 Prompt for AI Agents
In `@src/test/test_conformance.cpp` around lines 31 - 35, The test currently
hard-codes the plugin error code "1" when checking the reason for
TEST_RESULT_ERROR; update the assertion so it does not depend on a magic "1". In
the block that checks bpf_conformance_test_result_t::TEST_RESULT_ERROR (using
expected_result, reason, expected_reason), either parameterize the expected exit
code (e.g., expected_code) and build the expected message dynamically, or assert
that reason starts_with("Plugin returned error code ") and
ends_with(expected_reason) / contains the expected_reason suffix rather than
matching the full string; adjust the REQUIRE accordingly so the test accepts any
numeric exit code while still verifying the expected_reason.
Pull Request Test Coverage Report for Build 21309583909Details
💛 - Coveralls |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds RFC9669 conformance tests and updates the bpf_conformance submodule to include fixes for several test files.
Changes
Testing
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.