Skip to content
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

tests: drivers: uart: config api has extra dependency in test 2 #23459

Closed
jenmwms opened this issue Mar 13, 2020 · 7 comments · Fixed by #23509
Closed

tests: drivers: uart: config api has extra dependency in test 2 #23459

jenmwms opened this issue Mar 13, 2020 · 7 comments · Fixed by #23509
Assignees
Labels
area: Tests Issues related to a particular existing or missing test area: UART Universal Asynchronous Receiver-Transmitter Enhancement Changes/Updates/Additions to existing features

Comments

@jenmwms
Copy link
Collaborator

jenmwms commented Mar 13, 2020

Is your enhancement proposal related to a problem? Please describe.
Tests were added for the UART configure API in - tests: drivers: uart: config api #22849. The existing test implementation works, but the 2nd case: ”test_config_get()” will fail if we don’t run the 1st case :” test_uart_configure()” because the 2nd case have not set the value for the variable :”uart_cfg_check”.

Describe the solution you'd like
Suggest to make each case's independence as possible.

Describe alternatives you've considered
From comment in original discovery #22849 -

  • For the 1st case , we may only force on testing for setting configuration, so we may use the 1st case from your last version: 6c1156d to name test case: “test_uart_config_set()”.
  • For the 2nd case, we may use the 1st case from your new version: 721ce3a as 2nd test case, then name test case:”test_uart_config_get()”, and I have few comments below for the case’s last part.

Could you please help also review the code below ?
================last part code ==================
/* Verify configure() - set device configuration using data in cfg /
int ret = uart_configure(uart_dev, &uart_cfg);
zassert_ture(ret == 0, "set config error");

/ Confirm the values provided are the values set*/
ret = uart_config_get(uart_dev, &uart_cfg_check);
zassert_ture(ret == 0, "get config error");

if (memcmp(&uart_cfg, &uart_cfg_check, sizeof(uart_cfg)) != 0) {
return TC_FAIL;
} else {
return TC_PASS;
}
==============End last part code ==================

Additional context
tests: drivers: uart: config api #22849, included checking that the values stored and retrieved are as expected after calling the functions under test. The functions under test offer return values of success or error, which does not easily lend itself to check the stored/retrieved value in addition to the return value.

@jenmwms jenmwms added the Enhancement Changes/Updates/Additions to existing features label Mar 13, 2020
@jenmwms jenmwms self-assigned this Mar 13, 2020
@jenmwms
Copy link
Collaborator Author

jenmwms commented Mar 13, 2020

@xuhao8210 We can continue the discussion from #22849 here and resolve it with a PR. Thanks!

@jenmwms jenmwms changed the title tests: drivers: uart: config api has unintended dependency in test 2 tests: drivers: uart: config api has extra dependency in test 2 Mar 13, 2020
@jenmwms jenmwms added area: Tests Issues related to a particular existing or missing test area: UART Universal Asynchronous Receiver-Transmitter labels Mar 14, 2020
@xuhao8210
Copy link
Collaborator

@jenmwms OK. got it .Thanks Jen. if any comments for my suggestion, please let me know. Thanks again.

@jenmwms
Copy link
Collaborator Author

jenmwms commented Mar 16, 2020

@xuhao8210 Thanks for suggesting changes. I agree with the approach, however I have a comment about renaming the 1st case to test_uart_config_set(). There is not a config_set() in the uart configure API.

  • Is it breaking any test framework guidelines? IME the style is test_<api_function_name>().
  • Middle ground is test_uart_configure_set() so we still use the relevant function name but indicate something slightly different is happening in that test.

I'll submit a PR soon

@jenmwms
Copy link
Collaborator Author

jenmwms commented Mar 16, 2020

Why do we need to rename the first case? We have 2 test cases:

  1. test_uart_configure()
  2. test_uart_config_get()

@jenmwms
Copy link
Collaborator Author

jenmwms commented Mar 16, 2020

<For the 1st case , we may only force on testing for setting configuration, so we may use the 1st case from your last version: 6c1156d to name test case: “test_uart_config_set()”.>

So we do not (or cannot) verify the 1st test works (configure()) by checking a value? Because we need to use the partner function config_get() to retrieve the value we want to check effected by configure().

jenmwms added a commit to jenmwms/zephyr that referenced this issue Mar 16, 2020


Tests were added for the UART configure API in zephyrproject-rtos#22849. The existing test
implementation works, but suggested to reduce dependency between test
cases. This commit allows each test to run independently of each other
by removing the function call of uart_config_get() in the
uart_configure() test case, and uses the uart_config_get() test case to
confirm the value set and gotten/read.

Fixes zephyrproject-rtos#23459

Signed-off-by: Jennifer Williams <jennifer.m.williams@intel.com>
@xuhao8210
Copy link
Collaborator

@jenmwms For the test case name, you are right, and I forgot the name style, originally I would like to make the test case to better understand based on test case name. So please keep the test case name. Sorry for making you confuse. Thanks.

@xuhao8210
Copy link
Collaborator

@jenmwms For the 1st test case issue, if the 1st case is also include the checking point that it checks whether output value is same with the input value, the 1st case will same with 2nd case.
Personally I think that one case is enough because the 2nd case have covered the 1st case’s test point (e.g. refer the 2nd case in the PR: 23509), so I had ever given the comment : "we may merger the 2 test cases into one case , then add one checking point” in the PR: 22849.
But I think that it is also fine if there are 2 test cases for the test because I checked that other tests are also include 2 test cases. (e.g. “test_uart_fifo_read()” and “test_uart_fifo_fill()”), maybe we can keep the same style.
And I think that it is fine after I reviewed the newest PR: 23509. Thanks for your time to review my comments.

carlescufi pushed a commit that referenced this issue Mar 20, 2020
Tests were added for the UART configure API in #22849. The existing test
implementation works, but suggested to reduce dependency between test
cases. This commit allows each test to run independently of each other
by removing the function call of uart_config_get() in the
uart_configure() test case, and uses the uart_config_get() test case to
confirm the value set and gotten/read.

Fixes #23459

Signed-off-by: Jennifer Williams <jennifer.m.williams@intel.com>
hakehuang pushed a commit to hakehuang/zephyr that referenced this issue Apr 30, 2020


Tests were added for the UART configure API in zephyrproject-rtos#22849. The existing test
implementation works, but suggested to reduce dependency between test
cases. This commit allows each test to run independently of each other
by removing the function call of uart_config_get() in the
uart_configure() test case, and uses the uart_config_get() test case to
confirm the value set and gotten/read.

Fixes zephyrproject-rtos#23459

Signed-off-by: Jennifer Williams <jennifer.m.williams@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Tests Issues related to a particular existing or missing test area: UART Universal Asynchronous Receiver-Transmitter Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants