Conversation
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 introduces a new automated test case for the GNSI Acctz service. The purpose of this test is to ensure that the system correctly terminates the RecordSubscribe stream after an idle timeout period, validating the expected behavior of the device under test. 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 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 counter productive. 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
|
![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 introduces a new Go test for gNSI Acctz RecordSubscribe idle timeout functionality. The test aims to verify that the DUT closes the stream after an idle period. Review feedback highlights several issues: the client-side context timeout is currently shorter than the test's wait period, potentially causing false positives by timing out before the DUT; codes.DeadlineExceeded should not be considered an expected stream closure error as it indicates a client-side timeout; and the error from time.Parse for the system timestamp is ignored, which could lead to unexpected behavior.
|
|
||
| systemTime := gnmi.Get(t, dut, gnmi.OC().System().CurrentDatetime().State()) | ||
| startTime, _ := time.Parse(time.RFC3339, systemTime) | ||
| ctx, cancel := context.WithTimeout(t.Context(), defaultIdleTimeout) |
There was a problem hiding this comment.
The context timeout is set to defaultIdleTimeout (120s), but the test later sleeps for defaultIdleTimeout + gracePeriod (180s) at line 73. This causes the client-side context to expire before the sleep finishes, meaning Recv() at line 76 will always return context.DeadlineExceeded. This results in a false positive because the test isn't actually waiting for the DUT to close the connection; it's just hitting its own timeout. The context timeout should be significantly longer than the total wait time to ensure the DUT is the one initiating the closure.
| ctx, cancel := context.WithTimeout(t.Context(), defaultIdleTimeout) | |
| ctx, cancel := context.WithTimeout(t.Context(), defaultIdleTimeout+gracePeriod+30*time.Second) |
References
- In tests that inject artificial latency, convergence timeouts must be sufficiently long to account for the added delay and prevent premature test failure.
| } | ||
| if st, ok := status.FromError(err); ok { | ||
| switch st.Code() { | ||
| case codes.Canceled, codes.Unavailable, codes.DeadlineExceeded: |
There was a problem hiding this comment.
codes.DeadlineExceeded should be removed from the list of expected stream closure errors. If the RPC returns DeadlineExceeded, it indicates that the client-side context expired before the DUT closed the connection, which should be treated as a test failure (i.e., the DUT did not time out the idle session as expected).
| case codes.Canceled, codes.Unavailable, codes.DeadlineExceeded: | |
| case codes.Canceled, codes.Unavailable: |
| // acctz.SetupUsers(t, dut, false) | ||
|
|
||
| systemTime := gnmi.Get(t, dut, gnmi.OC().System().CurrentDatetime().State()) | ||
| startTime, _ := time.Parse(time.RFC3339, systemTime) |
There was a problem hiding this comment.
The error from time.Parse is ignored. If the DUT returns a timestamp in a non-RFC3339 format, startTime will be the zero value, which could lead to an invalid RecordRequest or unexpected DUT behavior. Use t.Errorf to report the error while allowing the test to continue.
| startTime, _ := time.Parse(time.RFC3339, systemTime) | |
| startTime, err := time.Parse(time.RFC3339, systemTime) | |
| if err != nil { | |
| t.Errorf("Failed to parse system time %q: %v", systemTime, err) | |
| } |
References
- In tests, prefer using t.Errorf over t.Fatalf to allow the test to run to completion and report all potential errors, rather than exiting on the first failure.
2 participants
No description provided.