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

Greentea Wifi testcase fixes #5568

Merged
merged 2 commits into from Dec 20, 2017

Conversation

Projects
None yet
5 participants
@VeijoPesonen
Contributor

VeijoPesonen commented Nov 23, 2017

Description

Greentea testcase WIFI-SCAN fails arbitrarily
Greentea testcase WIFI-SET-CREDENTIAL doesn't try WEP with valid credentials
Greentea testcase WIFI-GET-RSSI uses arbitrary RSSI value limits
Greentea testcase WIFI-CONNECT-PARAMS-VALID-SECURE assumes WPA2

Testcases in question either have unnecessary expectations about signal quality or make unnecessary assumptions about security features available. Testcases also assume that max password length is 64 characters when 802.11 says it's 63. We might have also access points with hidden SSIDs in vicinity but the scan test didn't like about those too much. This PR addresses all these issues.

Status

READY

Migrations

NO

Steps to test or reproduce

...\mbed-os> mbed test --compile -t ARM -m REALTEK_RTL8195AM --app-config .\TESTS\network\wifi\template_mbed_app.txt -n tests-network-wifi
...\mbed-os> mbedhtrun -f .\BUILD\tests\REALTEK_RTL8195AM\ARM\TESTS\network\wifi\wifi.bin -d X: -p COMY:9600
Greentea Wifi testcase fixes
ONME-3266 Greentea testcase WIFI-SCAN fails arbitrarily
ONME-3278 Greentea testcase WIFI-SET-CREDENTIAL doesn't try WEP with valid credentials
ONME-3279 Greentea testcase WIFI-GET-RSSI uses arbitrary RSSI value limits
ONME-3280 Greentea testcase WIFI-CONNECT-PARAMS-VALID-SECURE assumes WPA2
@VeijoPesonen

This comment has been minimized.

Contributor

VeijoPesonen commented Nov 23, 2017

@SeppoTakalo , @jarlamsa, Please review

@0xc0170 0xc0170 requested review from SeppoTakalo and jarlamsa Nov 23, 2017

@SeppoTakalo

Otherwise correct changes but the connect_params_valid_secure() does not seem to make sense anymore.

It is not following the test plan anymore.

Unfortunately we need to have tradeoff and test only against the most common, WPA2, setup. We don't have resources enough to build access point for all.

if(wifi->connect(MBED_CONF_APP_WIFI_SECURE_SSID, MBED_CONF_APP_WIFI_PASSWORD, NSAPI_SECURITY_WEP) == NSAPI_ERROR_OK) {
TEST_PASS();
}

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Nov 23, 2017

Contributor

I don't think this works.

Our test environment does not have SSID that would allow you to connect with all these parameters.

You can only expect one to work.

Also, if first one works, how can we expect second connect() to work without disconnecting in between.

This comment has been minimized.

@VeijoPesonen

VeijoPesonen Nov 24, 2017

Contributor

Based on the fact that test case execution should abort immediately when TEST_PASS is called there shouldn't be an issue. WPA2 is tried first so this should behave the same in our environment as the previous solution.

/* It is not necessary for you to call PASS. A PASS condition is assumed if nothing fails.
 * This method allows you to abort a test immediately with a PASS state, ignoring the remainder of the test. */
#define TEST_PASS()    longjmp(Unity.AbortFrame, 1)

If connect() fails and is repeated with another security protocol this shouldn't be an issue as the failed try should only report error - which gets ignored. There shouldn't be any side-effects. Only if none of the methods succeeds we have an error situation. Lets also remember that these test cases are not only to be run in our test environment.

Also a thing to keep in mind is that WIFI-SET-CREDENTIAL test case checks that all the expected security protocols are supported.

This comment has been minimized.

@VeijoPesonen

VeijoPesonen Nov 30, 2017

Contributor

Our test environment does not have SSID that would allow you to connect with all these parameters.

Our test environment gets covered by the first if-clause, others are to prevent the need to modify the test case itself when someone else is using an access point with WEP/WPA/WPA_WPA2 Mixed mode.

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Nov 30, 2017

Contributor

Actually, can you make if like:

if (...) {
    TEST_PASS();
    return;
}

just to make it more obvious.

This comment has been minimized.

@VeijoPesonen

VeijoPesonen Dec 1, 2017

Contributor

That's as simple as it can get. TEST_PASS replaced with return. test cases will be marked as passed if there is nothing which would mark them as failed.

@0xc0170 0xc0170 requested a review from kjbracey-arm Nov 30, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 30, 2017

What about ODIN target testing for this patch?

Greentea Wifi testcase fixes
Replace 'TEST_PASS' with 'return'
@VeijoPesonen

This comment has been minimized.

Contributor

VeijoPesonen commented Dec 1, 2017

Not tried with ODIN as I don't have an access point with which to try at the moment. The ODIN driver won't accept WPA2 - it requires WPA/WPA2. @andreaslarssonublox mentioned in comments of #5582 that WPA2 isn't and won't be supported for now.

@VeijoPesonen

This comment has been minimized.

Contributor

VeijoPesonen commented Dec 8, 2017

Would someone please merge these changes - I would like to create a new PR but those new modifications are build on top of these.

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Dec 8, 2017

@0xc0170 These are ready to go in.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 8, 2017

@VeijoPesonen For future, please provide better headlines for a commit msg where possible, Greentea Wifi testcase fixes having in the log 100x does not provide meaningful information. It's fine with this patch, there are small bugfixes, and just 2 commits that have details in

/morph build

@VeijoPesonen

This comment has been minimized.

Contributor

VeijoPesonen commented Dec 8, 2017

@0xc0170 yes, good point.

@mbed-ci

This comment has been minimized.

mbed-ci commented Dec 8, 2017

Build : SUCCESS

Build number : 671
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5568/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@VeijoPesonen

This comment has been minimized.

Contributor

VeijoPesonen commented Dec 13, 2017

"ci-morph-test — tests not started" - What is the trigger for these tests?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 13, 2017

/morph test

@mbed-ci

This comment has been minimized.

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Dec 18, 2017

@0xc0170 Please merge in.

@0xc0170 0xc0170 merged commit 63a9237 into ARMmbed:master Dec 20, 2017

10 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2 Local mbed2 testing has passed
Details
travis-ci/tools Local tools testing has passed
Details

@VeijoPesonen VeijoPesonen deleted the VeijoPesonen:greentea-wifi-fixes branch Jan 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment