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
Fix for configurable network driver tests #5335
Conversation
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.
This is great! I have just left some nits below.
tools/test_configs/__init__.py
Outdated
@@ -10,7 +11,10 @@ def get_valid_configs(target_name): | |||
if target_name in TARGET_CONFIGS: | |||
target_config = TARGET_CONFIGS[target_name] | |||
else: | |||
return {} | |||
if 'LWIP' in TARGET_MAP[target_name].features: |
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.
Can this be an elif
instead? that would make the code a smidgen easier to follow.
tools/test_configs/__init__.py
Outdated
@@ -31,5 +35,11 @@ def get_default_config(target_name): | |||
if config_name == "NONE": | |||
return None | |||
return join(CONFIG_DIR, CONFIG_MAP[config_name]) | |||
elif target_name in TARGET_MAP: | |||
if 'LWIP' in TARGET_MAP[target_name].features: |
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.
Since the else
following both the elif
above and the if
here have the same behavior, could this if
be folded into the elif
? for example:
elif (target_name in TARGET_MAP and
'LWIP' in TARGET_MAP[target_name].features):
return join(CONFIG_DIR, CONFIG_MAP["ETHERNET"])
@theotherjimmy Made those changes 😄 |
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.
Looks great!
@yennster - Can you go ahead and remove the special case for K64F - https://github.com/ARMmbed/mbed-os/blob/master/tools/test_configs/target_configs.json#L6-L8 |
@sarahmarshy done 👍 |
@theotherjimmy, this needs to go in before any other network prs. Currently network tests are only running for K64F in CI. @yennster nice fix 👍 |
@studavekar You also needed to know about the priority here, but it looks like you were paying close attention anyway. 😄 |
Build : SUCCESSBuild number : 206 Triggering tests/morph test |
Test : FAILUREBuild number : 100 |
There is one failure for ARCH_PRO + IAR : tests-netsocket-socket_sigio log link
|
@studavekar The fix for the tests-netsocket-socket_sigio test should come in a separate PR |
Build : SUCCESSBuild number : 214 Triggering tests/morph test |
Test : FAILUREBuild number : 104 |
This is then blocked on proceeding fix. I'll label it as such. @yennster @sarahmarshy Can you fix tests-netsocket-socket_sigio ? I haven't spotted it in the patches. |
OK, ethernet becomes the default configuration, |
@0xc0170 Sorry for the delay, I am looking into the tests-netsocket-socket_sigio test for arch_pro now |
@0xc0170 Unfortunately, I cannot find another arch_pro board in the office to replicate the failure locally, I look around again on Tuesday |
I take it #4795 was the preceding PR for this one (which has now been merged) ? |
/morph test |
Test : FAILUREBuild number : 152 |
Hi |
@yennster Could you rebase this PR to resolve the conflicts? |
682979c
to
141c030
Compare
141c030
to
fbdd019
Compare
@theotherjimmy Rebased against master. |
/morph build |
Build : SUCCESSBuild number : 529 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 142 |
Test : SUCCESSBuild number : 335 |
@jeromecoutant, these issues are unrelated to this PR. Can you please file an issue for your concerns? |
@theotherjimmy Is this good to go in? |
Probably? @geky Could you give this a quick re-review? There are a few commits since your last review. |
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 don't really follow much of the tools development, but the checkmark's green and the tests are passing so
LGTM!
Hi |
Please check #5576 |
Description
This change fixes a bug with @sarahmarshy's configurable network driver tests. These tests can be invoked with an existing interface inside mbed OS or a new NSAPI implementation via:
mbed test -m [MCU] -t gcc_arm -n mbed-os-tests-netsocket* --test-config ethernet
or
mbed test -m [MCU] -t gcc_arm -n mbed-os-tests-netsocket* --test-config path\to\config\file
Bug fixed allows for any board with LWIP (not just K64F or Odin) to run mbed OS network tests. Also fixed a comment that describes what the
get_test_config
function returns.Edit:
Also,
mbed test -m [MCU] -t gcc_arm -n mbed-os-tests-netsocket*
will by default choose the Ethernet interface for any target with LWIP.Related PRs
@sarahmarshy @theotherjimmy