Update P4rt-5.3#5499
Conversation
AmrNJ
commented
May 22, 2026
AmrNJ
commented
- Increased packet-in timeouts from 30s to 45s.
- Added 1% tolerance for packet count received as the traffic distribution verification function later anyways have 2% tolerance. This avoids the test from failing early.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the P4rt-5.3 test suite to enhance reliability. By extending timeout windows and introducing a margin of error for packet counts, the changes prevent transient network fluctuations from causing premature test failures. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Pull Request Functional Test Report for #5499 / 70cd30eVirtual Devices
Hardware Devices
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the testPacketIn function to implement a tolerance-based check (1%) for the number of received packets instead of an exact match, and increases the packet retrieval timeout to 45 seconds. Review feedback indicates that the error from StreamChannelGetPackets is now being ignored, which should be handled with t.Fatalf to ensure the test fails immediately upon connection loss. Additionally, it was suggested to define the tolerance value as a named constant to avoid the use of magic numbers.
| if err != nil { | ||
| t.Errorf("Unexpected error on fetchPackets: %v", err) | ||
| } | ||
| _, packets, _ := test.client.StreamChannelGetPackets(&streamName, uint64(test.wantPkts), 45*time.Second) |
There was a problem hiding this comment.
The error returned by StreamChannelGetPackets is now explicitly ignored using _. This can hide critical failures like connection loss. Per repository rules, t.Fatalf should be used instead of t.Errorf because a failure to retrieve packets makes the subsequent tolerance checks meaningless. This ensures the test fails fast.
_, packets, err := test.client.StreamChannelGetPackets(&streamName, uint64(test.wantPkts), 45*time.Second)
if err != nil {
t.Fatalf("Unexpected error on fetchPackets: %v", err)
}References
- In tests, t.Fatalf is preferred over t.Errorf when a failure makes subsequent test steps meaningless, as this fails fast and reduces overall test execution time.
| gotPkts := 0 | ||
| t.Logf("Start to decode packet and compare with expected packets.") | ||
| wantPacket := args.packetIO.GetPacketTemplate() | ||
| tolerance := 0.01 |
There was a problem hiding this comment.
The tolerance value 0.01 is a magic number. It is better to define it as a named constant or add a comment explaining why this specific value was chosen. If this value represents a limitation due to external factors, it can be retained as a magic number provided there is a comment explaining the condition.
| tolerance := 0.01 | |
| const tolerance = 0.01 |
References
- Magic numbers that represent temporary fixes or limitations due to external factors can be retained in the code with appropriate comments explaining their temporary nature.
2 participants
