Skip to content

[PATCH v1] validation: fix classification test init #2282

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.

Sign up for GitHub

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

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

[PATCH v1] validation: fix classification test init #2282

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions test/validation/api/classification/odp_classification_test_pmr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ static test_state_t test_init(cls_flag_t cls)

ts.pktio = create_pktio(ODP_QUEUE_TYPE_SCHED, pkt_pool, cls == ENABLE_CLS);
CU_ASSERT_FATAL(ts.pktio != ODP_PKTIO_INVALID);
CU_ASSERT(odp_pktin_event_queue(ts.pktio, &ts.pktin_queue, 1) == 1);
CU_ASSERT(odp_pktin_event_queue(ts.pktio, &ts.pktin_queue, 1) > 0);
Copy link
Collaborator

@JannePeltonen JannePeltonen Oct 21, 2025

Choose a reason for hiding this comment

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

The assertion checks for exactly one queue. Changed this to check for non-zero number of queues.

Why? What is wrong with the current code? If there is a valid reason for the change (but I cannot immediately see it), the commit message could say it.

Copy link
Collaborator

@JannePeltonen JannePeltonen Oct 21, 2025

Choose a reason for hiding this comment

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

This assertion to check for exactly one queue was added recently. Before this, on cn9k, cn10k classificati>on tests have been working fine with 64 queues. This change breaks them. Is there any reason the tests need to run with exactly one queue?

Failure of a validation test may be caused by a bug in the validation test or by a bug in the SW under testing and therefore a test failure alone is not a sufficient reason to change a validation test. If we took care of all test failures by changing the tests, then the tests would be useless.

If an API validation test does not have bugs and uses the API correctly, then a test failure implies that there is problem in the SW being tested. It may also be that the failure exposes a problem in the API itself, but then the API needs a fix too, not just the test.

Looking at the code, test_init() calls create_pktio() which creates a pktio and configures it with the default number of pktin queues, i.e. 1. So checking then that odp_pktin_event_queue() returns exactly 1 seems right to me.

So why are you proposing this change?

CU_ASSERT(start_pktio(ts.pktio) == 0);

configure_default_cos(ts.pktio, &ts.default_cos, &ts.default_queue, &ts.default_pool);
Expand All @@ -150,7 +150,7 @@ static uint32_t send_packet(odp_packet_t pkt, odp_pktio_t pktio)
{
uint32_t seqno;
odph_ethhdr_t *eth;
uint32_t len;
uint32_t len = 0;

seqno = cls_pkt_get_seq(pkt);
CU_ASSERT(seqno != TEST_SEQ_INVALID);
Expand Down
Loading