-
Notifications
You must be signed in to change notification settings - Fork 444
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
[P4Testgen] Implement coverage tracking of actions #4307
Conversation
# now with --track-coverage STATEMENTS --track-coverage ACTIONS
$ ./p4testgen --target bmv2 --test-backend STF ../backends/p4tools/modules/testgen/targets/bmv2/test/p4-programs/bmv2_table_actions_coverage.p4 --max-tests 256 --path-selection GREEDY_STATEMENT_SEARCH --only-covering-tests --stop-metric MAX_NODE_COVERAGE --track-coverage STATEMENTS --track-coverage ACTIONS
warning: Ingress parser exception handler not fully implemented
============ Test 0: Nodes covered: 0.526316 (10/19) ============
============ Test 1: Nodes covered: 0.684211 (13/19) ============
============ Test 2: Nodes covered: 0.736842 (14/19) ============
============ Test 3: Nodes covered: 0.842105 (16/19) ============
============ Test 4: Nodes covered: 0.894737 (17/19) ============
============ Test 5: Nodes covered: 0.947368 (18/19) ============
============ Test 6: Nodes covered: 1 (19/19) ============
# and without (with just STATEMENTS)
$ ./p4testgen --target bmv2 --test-backend STF ../backends/p4tools/modules/testgen/targets/bmv2/test/p4-programs/bmv2_table_actions_coverage.p4 --max-tests 256 --path-selection GREEDY_STATEMENT_SEARCH --only-covering-tests --stop-metric MAX_NODE_COVERAGE --track-coverage STATEMENTS
warning: Ingress parser exception handler not fully implemented
============ Test 0: Nodes covered: 0.777778 (7/9) ============
============ Test 1: Nodes covered: 0.888889 (8/9) ============
============ Test 2: Nodes covered: 1 (9/9) ============ |
4705519
to
4786284
Compare
@@ -184,6 +184,7 @@ const IR::Expression *TableStepper::evalTableConstEntries() { | |||
const auto *actionType = stepper->state.getP4Action(tableAction); | |||
auto &nextState = stepper->state.clone(); | |||
nextState.markVisited(entry); | |||
nextState.markVisited(actionType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is actually a very specific type of coverage. This does not cover when an action is called directly and not via a table, for example. Is this intended?
Otherwise, I would just move the markVisited call into the P4Action visitor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I'm not sure how the testgen's midend is configured and if there will be directly-called actions left, I should check it (I presumed there will be none). If there can be actions called directly, it would make sense to handle those too probably (presumably they would be still included in statements coverage because of the call, but it would still make sense to have them in actions coverage too).
A slight problem is that I actually have to mark the action as visited before the step into it is generate as in many places the table stepper creates a new, cloned action out of the original, with the arguments given by the control plane config;
p4c/backends/p4tools/modules/testgen/core/small_step/table_stepper.cpp
Lines 346 to 348 in 4786284
// We add the arguments to our action call, effectively creating a const entry call. | |
auto *synthesizedAction = tableAction->clone(); | |
synthesizedAction->arguments = arguments; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, MoveActionsToTables
is not running in testgen, let me think about a way to handle directly called actions too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A slight problem is that I actually have to mark the action as visited before the step into it is generate as in many places the table stepper creates a new, cloned action out of the original, with the arguments given by the control plane config;
That should not be a problem for the markVisited function since we are comparing entries based on the valid source information? If we mismatch or add duplicate entries that looks like a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I did not realize we use the source info for identity. In that case this is actually quite simple.
backends/p4tools/modules/testgen/lib/collect_coverable_nodes.cpp
Outdated
Show resolved
Hide resolved
I've changed this to track all actions calls (I've also changed the test). # now with --track-coverage STATEMENTS --track-coverage ACTIONS
$ ./p4testgen --target bmv2 --test-backend STF ../backends/p4tools/modules/testgen/targets/bmv2/test/p4-programs/bmv2_table_actions_coverage.p4 --max-tests 256 --path-selection GREEDY_STATEMENT_SEARCH --only-covering-tests --stop-metric MAX_NODE_COVERAGE --track-coverage STATEMENTS --track-coverage ACTIONS
warning: Ingress parser exception handler not fully implemented
============ Test 0: Nodes covered: 0.461538 (12/26) ============
============ Test 1: Nodes covered: 0.653846 (17/26) ============
============ Test 2: Nodes covered: 0.692308 (18/26) ============
============ Test 3: Nodes covered: 0.807692 (21/26) ============
============ Test 4: Nodes covered: 0.846154 (22/26) ============
============ Test 5: Nodes covered: 0.923077 (24/26) ============
============ Test 6: Nodes covered: 0.961538 (25/26) ============
============ Test 7: Nodes covered: 1 (26/26) ============
# and without (with just STATEMENTS)
$ ./p4testgen --target bmv2 --test-backend STF ../backends/p4tools/modules/testgen/targets/bmv2/test/p4-programs/bmv2_table_actions_coverage.p4 --max-tests 256 --
path-selection GREEDY_STATEMENT_SEARCH --only-covering-tests --stop-metric MAX_NODE_COVERAGE --track-coverage STATEMENTS
warning: Ingress parser exception handler not fully implemented
============ Test 0: Nodes covered: 0.615385 (8/13) ============
============ Test 1: Nodes covered: 0.692308 (9/13) ============
============ Test 2: Nodes covered: 0.846154 (11/13) ============
============ Test 3: Nodes covered: 0.923077 (12/13) ============
============ Test 4: Nodes covered: 1 (13/13) ============ Note that for freestanding actions, the action calls are statements so they are actually included in the |
Co-authored-by: Fabian Ruffy <5960321+fruffy@users.noreply.github.com>
Resolves #4305.
This PR adds a new coverage tracking option for P4Testgen:
--track-coverage ACTIONS
. This makes it possible to track coverage of action invocation, including empty actions which are not included in the statement coverage because they don't contain any statements.I've added a test that exercises this feature and that I have used to test it manually. However, I don't see a way we can currently automatically test that this testgen will generate all the right tests for this test case. This could be tested better if/when #4304 is implemented.