Repository navigation
Conversation
… tests When eth_config header.version >= 5, the link timeout closure tests several invalid values (1000, 31000, 2345) to verify ValueError is raised. However, new_config.link.timeout was left at 2345 (invalid) after the closure, causing every subsequent test closure (MTU, transmission delay, UDP TTL) to fail with: ValueError: Link timeout must be divisible by 100. Current 2345 This was a latent bug that became visible when io-drivers PR realsenseai#172 bumped ETH_CONFIG_VERSION from 4 to 5, activating the previously unreachable version-5 branch. Fix: restore new_config.link.timeout to orig_config.link.timeout at the end of the link timeout closure, consistent with how MTU, transmission_delay, and udp_ttl closures already restore their fields.
|
Can one of the admins verify this patch? |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a test isolation bug in test-eth-config.py where the “Test link timeout configuration” closure (for ETH_CONFIG_VERSION >= 5) leaves new_config.link.timeout set to an intentionally-invalid value after validation tests, causing subsequent closures to fail validation before reaching their target fields.
Changes:
- Restore
new_config.link.timeouttoorig_config.link.timeoutat the end of the link-timeout closure to prevent cascading failures in later tests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pinquanw
approved these changes
Apr 15, 2026
OhadMeir
approved these changes
Apr 15, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
test-eth-config.pyfails on three tests after PR soc-rs-soc-io-drivers#172 bumpsETH_CONFIG_VERSIONfrom 4 → 5:All fail with:
Root Cause
The
Test link timeout configurationclosure, whenheader.version >= 5, tests invalid timeout values including2345. After eachset_eth_config()call raisesValueError, the exception path is caught correctly—butnew_config.link.timeoutis left at 2345 (never restored).Previously this code path was unreachable (
header.versionwas 4), so the bug was dormant. With version 5, the block is now executed, leaving the config object in an invalid state for all subsequent closures.Every later closure calls
set_eth_config(new_config)→build_command()→validate(), which rejects the stale invalidlink.timeoutbefore reaching the field each test actually changes.Fix
Add a restore line at the end of the link timeout closure, consistent with the pattern used by MTU, transmission_delay, and udp_ttl closures:
Note: This is a latent bug in the test, not in the firmware or the io-drivers PR itself.