Icetea support #7745

OPpuolitaival · 2018-08-09T14:41:37Z

Description

Add icetea support including tools code and couple of example tests

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Feature
[ ] Breaking change

0xc0170

My 4 comments are just because the location of entire patch here is wrong. Shouldn't this in frameworks where other test tools are?

0xc0170 · 2018-08-10T11:42:31Z

Makefile

@@ -0,0 +1,5 @@
+SRCS := $(wildcard source/*.c)


I dont think we want to have this in the root. makefile that is just for cmdline tool

Also yotta_ignore file

0xc0170 · 2018-08-10T11:43:07Z

.mbedignore

@@ -0,0 +1,2 @@
+test/*
+source/ns_list_internal/*


source path does not look correct, there's o source/ in the root here as .mbedignore is

0xc0170 · 2018-08-10T11:43:26Z

.mbedignore

@@ -0,0 +1,2 @@
+test/*


where this comes from tests/ ?

All these comments are from one git subtree try.. I will fix these

OPpuolitaival · 2018-08-10T12:43:35Z

@0xc0170 I will put mbed-client-cli in better place on monday when I can get support from @jarlamsa

ithinuel

Great job :)

I only reviewed the python scripts. From what I understood they look good to me.

I have some concerns regarding tools/test_api.py that contains a lot of changes related to white spaces and empty lines that should probably not be part of this PR and few other little things.

ithinuel · 2018-08-13T14:59:11Z

tools/test.py

 """
 from __future__ import print_function, division, absolute_import
 import sys
 import os
-import json
+import subprocess


subprocess is not used.

ithinuel · 2018-08-13T15:01:26Z

tools/toolchains/__init__.py

@@ -1117,4 +1117,4 @@ def report(self):
    u'IAR': IAR
 }

-TOOLCHAINS = set(TOOLCHAIN_CLASSES.keys())
+TOOLCHAINS = set(TOOLCHAIN_CLASSES.keys())


this is only removing a linefeed.

ithinuel · 2018-08-13T15:02:21Z

tools/test_api.py

@@ -28,14 +28,17 @@
 import random
 import argparse
 import datetime
+import errno


errno is not used in this file.

ithinuel · 2018-08-13T15:02:22Z

tools/test_api.py

 import threading
 import ctypes
 import functools
+import subprocess


subprocess is not used in this file.

ithinuel · 2018-08-13T15:14:06Z

tools/test/run_icetea/TEST_DIR_HW/test_predefined_platforms.py

+
+class Testcase(Bench):
+    def __init__(self):
+        Bench.__init__(self,


Same as previous comment in tools/test/run_icetea/TEST_DIR/test_pass.py.

ithinuel · 2018-08-13T15:14:19Z

tools/test/run_icetea/TEST_DIR_HW/test_K64F.py

+
+class Testcase(Bench):
+    def __init__(self):
+        Bench.__init__(self,


Same as previous comment in tools/test/run_icetea/TEST_DIR/test_pass.py.

ithinuel · 2018-08-13T15:14:42Z

tools/test/run_icetea/TEST_DIR/test_print.py

+
+class Testcase(Bench):
+    def __init__(self):
+        Bench.__init__(self,


Same as previous comment in tools/test/run_icetea/TEST_DIR/test_pass.py.

ithinuel · 2018-08-13T15:14:49Z

tools/test/run_icetea/TEST_DIR/test_pass.py

+
+class Testcase(Bench):
+    def __init__(self):
+        Bench.__init__(self,


I'm not 100% sure about this, but I usually see super() used in this case instead of invoking explicitly the Base class.

I tried super but did not work for some reason

ithinuel · 2018-08-13T15:33:17Z

requirements.txt

@@ -6,7 +6,7 @@ IntelHex>=1.3
 junit-xml
 pyYAML
 requests
-mbed-ls>=0.2.13
+mbed-ls>=0.2.13,<2.0


Why do we add a high limit to mbed-ls version ?

This is syncronized with icetea. If not then there is risk that icetea and this support different version and cause problems

Is this because of https://github.com/ARMmbed/icetea/blob/929dc1da4c2529fff633545c00b677d0970ee0a5/setup.py#L32
It seems we actually also need a version >=1.4.2

mbed-ls v0.2.13 is very old - maybe it's better to update this anyway..

jupe · 2018-08-18T18:26:48Z

TEST_APPS/device/exampleapp/main.cpp

+}
+
+FileHandle* mbed::mbed_override_console(int) {
+    static UARTSerial console(STDIO_UART_TX, STDIO_UART_RX, 115200);


baudrate could be defined beginning of file

jupe · 2018-08-18T18:27:36Z

TEST_APPS/device/exampleapp/main.cpp

+	int c;
+	while((c = getchar()) != EOF) {
+        cmd_char_input(c);
+    }


nitpick: wrong indent - check whole file

jupe · 2018-08-18T18:30:00Z

TEST_APPS/device/socket_app/cmd_ifconfig.cpp

+const char* MAN_IFCONFIG = "  ifup      interface up\r\n"\
+                     "  ifdown      interface down\r\n";
+
+static bool is_ipv4(const char *str)


isn’t there any better/more robust way to detect whether str is ipv4 or 6?

jupe · 2018-08-18T18:31:49Z

TEST_APPS/device/socket_app/cmd_ifconfig.cpp

+static void ifconfig_print()
+{
+    if(!net)
+        return;


maybe here should be print that no interface configured etc

jupe · 2018-08-18T18:34:06Z

TEST_APPS/device/socket_app/cmd_ifconfig.cpp

+
+
+
+const char* networkstack_error_to_str(int errorcode)


this looks something (also) that could be shared/needed between (cli)apps, could we have shared library which contains commons stuff for testapps?

Can be done in the future when there are multiple apps. Basically the whole command could be common stuff, but where do we draw the line that what is common between apps and what is not.

jupe · 2018-08-18T19:43:53Z

tools/run_icetea.py

+            if options.verbose:
+                cmd(['icetea', '--tcdir', options.tcdir, '--list'] + (['-v'] if options.verbose else []))
+
+            cmd(['icetea', '--tcdir', options.tcdir, '--suite', options.test_suite, '--clean', '--plugin_path',


looks that we doesn't have any way to pypass other optional icetea arguments here, any plan/chance to make it possible ?

Current idea is that advanced usage will happen without these scripts

How we bring remote resource support in future then? Do we bring parameters one by one when needed? Doesn’t sound good approach either 🙄

jupe · 2018-08-18T19:45:09Z

tools/test/run_icetea/.gitignore

@@ -0,0 +1 @@
+test_suite.json


nitpick: missing empty line (same issue seems to be in multiple files)

jupe · 2018-08-18T19:46:37Z

tools/test/run_icetea/TEST_DIR/test_print.py

@@ -0,0 +1,37 @@
+"""


do we need this file at all? what about previous one (test_pass.py) ?

jupe · 2018-08-18T19:47:17Z

tools/test/run_icetea/TEST_DIR_HW/test_K64F.py

+    def __init__(self):
+        Bench.__init__(self,
+                       name="test_K64F_only",
+                       title="Test a test case which have onely K64F support",


nitpick: typo

jupe · 2018-08-18T19:48:16Z

tools/test/run_icetea/run_icetea_test.py

@@ -0,0 +1,104 @@
+"""
+mbed SDK
+Copyright (c) 2016 ARM Limited


jupe · 2018-08-20T14:12:41Z

TEST_APPS/device/socket_app/cmd_socket.cpp

@@ -299,9 +314,6 @@ static void sigio_handler(SInfo *info) {
 void cmd_socket_init(void)
 {
    cmd_add("socket", cmd_socket, "socket", MAN_SOCKET);
-    cmd_alias_add("socket help", "socket -h");
-    cmd_alias_add("socket --help", "socket -h");


maybe last one would be still usefull - does test uses it?

That one seems to be an artifact from porting. It isn't used nor implemented.

Dismissed. Reviewing again

0xc0170 · 2018-08-20T15:43:18Z

TEST_APPS/device/exampleapp/main.cpp

+    cmd_next(retcode);
+}
+
+void wrap_printf(const char *f, va_list a) {


Align the code (run astyle on these example apps)

0xc0170 · 2018-08-20T15:46:47Z

TEST_APPS/readme.md

+
+Now that the interface has been selected you can run the icetea tests from the mbed-os root on your command line by
+
+`>mbed test -m <target> -t <toolchain> --icetea`


I asked in the unittest PR, just to be sure this is fine also here (commands/arguments alignment for cli) - how we can test (unit/icetea/app tests) - using one command mbed test and --argument.

This is handled in unit testing.. it add new command "mbed unittest" because it is so different than icetea or greentea

Another round

0xc0170 · 2018-08-24T12:26:07Z

@ARMmbed/mbed-os-tools Please review tools changes

Documentation addition ARMmbed/mbed-os-5-docs#659
mbed cli addition - ARMmbed/mbed-cli#731

bridadan

Mostly looks good! Some doc issues and a few implementation questions but overall seems to be in good shape.

bridadan · 2018-08-27T16:45:42Z

TEST_APPS/readme.md

+
+### Structure
+
+mbed-os has a folder called TEST_APPS that contains everything related to IceTea -testing.


nit: you can drop the - in -testing.

bridadan · 2018-08-27T16:46:38Z

TEST_APPS/readme.md

+- icetea-plugins - contains plugins that are being used by some of the testcases, needed for the test execution
+- testcases - contains IceTea testcases written in Python
+
+The testcases and test applications have a dependency


Should there be something more in this sentence? What is the dependency?

bridadan · 2018-08-27T16:47:09Z

TEST_APPS/readme.md

+### Structure
+
+mbed-os has a folder called TEST_APPS that contains everything related to IceTea -testing.
+There are currently 3 folders


Typically a : should go after folders I think. Like:

There are currently 3 folders:

bridadan · 2018-08-27T16:47:45Z

TEST_APPS/readme.md

+
+#### Prerequisities
+
+You should have IceTea and forked mbed-cli that supports IceTea, installed.


I think we want to remove forked from this sentence, considering that we'll be merging this into a released version of Mbed CLI.

bridadan · 2018-08-27T16:51:19Z

TEST_APPS/readme.md

+```
+
+The results from the tests can also be found from mbed-os/log -folder.
+You probably want to add the log -folder to your .mbedignore -file to prevent issues with build commands becoming too long over the time.


There's a few stray - characters in these sentences, I'd remove them for clarity.

bridadan · 2018-08-27T16:52:31Z

requirements.txt

@@ -15,3 +15,4 @@ pyelftools>=0.24
 jsonschema>=2.6
 future>=0.16.0
 six>=1.11.0
+icetea==1.0.0rc3


Looks like this is using an rc release, is there a non-rc release we should be using?

I don’t want to tag release before everything is ready, including OOB’s.. There is at least one doc PR landing to icetea before release.. But yes, eventually we have 1.0.0 release :)

This will be change when release of icetea is published

Ok, will icetea be released to 1.0.0 before this PR is merged? Is there an order to PRs that the maintainers need to be aware of?

@bridadan FYI, it looks like the most recent commit updates this: c6c323d

Not sure why GitHub is still showing this.

bridadan · 2018-08-27T16:53:30Z

tools/build_api.py

                        build[0]['elf'] = relpath(build[0]['elf'], path_to_file)
                        build[0]['bin'] = relpath(build[0]['bin'], path_to_file)
                    except KeyError:
                        pass
                    if 'type' not in build[0]:
                        build[0]['type'] = app_type
-                    build_data['builds'].append(build[0])
-    dump(build_data, open(filename, "w"), indent=4, separators=(',', ': '))
+                    build_data['builds'].insert(0, build[0])


Any particular reason for this change?

The build_data.json is not cleared and it will contain also failed builds. Previously the latest build was on the bottom of the build_data.json. This appends the build_data from the start so the latest build is in the top of the build_data.json. The run_icetea.py is looking this file to get all the test application binary paths and apparently read only the first build result. Other option to get the latest build is to start looking for the build date and which one is the latest, not sure if its reliable in every case. The date seems to be coming from python datetime method. (datetime.utcnow().isoformat())

Sorry for being slow, can you explain this sentence a bit more?

The run_icetea.py is looking this file to get all the test application binary paths and apparently read only the first build result.

Just curious why the order is important in this particular case, this isn't a blocker issue though.

Previously if your first build failed, the failed build data was in the top of build_data.json, if the run_icetea.py only read this failed one and tried to get the test application binary paths from this, all the tests would fail because there was no binary path found. This would also happen in the future test runs if you don't use -c or delete the build_data.json separately.
Now the script also checks that was the build successful or not.

Ok I see, sounds like a workaround then. Glad to hear the updated version checks the result! Thanks for the explanation

bridadan · 2018-08-27T19:24:18Z

tools/run_icetea.py

+    for test in icetea_json_output:
+        skip = False
+
+        for dut in test['requirements']['duts'].values():


I think it'd be helpful to have some warnings displayed if possible in this case.

donatieng · 2018-08-28T10:58:03Z

requirements.txt

@@ -15,3 +15,4 @@ pyelftools>=0.24
 jsonschema>=2.6
 future>=0.16.0
 six>=1.11.0
+icetea==1.0.0rc4


Seems very restrictive

I would not rely only on rc4 version.
Looking at other requirements >=1.0.0 would be fine?

0xc0170 · 2018-08-29T11:17:44Z

Can you rebase? Once done, let us know

OPpuolitaival · 2018-08-29T14:23:53Z

Rebased and works in my tests now.. @jarlamsa can you check that works in windows and your setup too

cmonr · 2018-08-30T03:50:43Z

@0xc0170 @bridadan @jupe @ithinuel @jarlamsa @donatieng @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-test If y'all could take a quick look in the next 12-24 hrs, that would be wonderful.

jarlamsa · 2018-08-30T06:57:59Z

@OPpuolitaival seems to be working now

theotherjimmy

A dependency of Icetea, psutil, requires a native C/C++ compiler to install through pip. Please remove this dependency.

theotherjimmy

Icetea 1.0.1 does not have native-compiler dependencies. CI should work now.

theotherjimmy · 2018-08-31T20:02:14Z

@cmonr I restarted travis. Let's get this tested.

cmonr · 2018-08-31T23:50:11Z

/morph build

mbed-ci · 2018-09-01T00:55:53Z

Build : FAILURE

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

cmonr · 2018-09-01T04:21:22Z

I think this is still dependent on #7814

OPpuolitaival · 2018-09-01T07:25:45Z

@SeppoTakalo any idea why emac test build fails? Should not be related to this PR:
[DEBUG] Output: "./TESTS/network/emac/main.cpp", line 21: Error: #35: #error directive: [NOT_SUPPORTED] Requires parameters from mbed_app.json

0xc0170 · 2018-09-01T08:30:41Z

Where do they come from? No supported is fine (should not cause CI to report failure, neither a user) , just a test that requires some additional info (no default provided )

the failures in the build examples come from connectivity PR (will be fixed asap)

cmonr · 2018-09-01T22:40:17Z

#7814 is now in! Let's see if this improves things.
/morph build

mbed-ci · 2018-09-01T23:50:29Z

Build : SUCCESS

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

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-09-02T01:42:39Z

Test : SUCCESS

Build number : 2740
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7745/2740

mbed-ci · 2018-09-02T04:08:25Z

Exporter Build : SUCCESS

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

cmonr added the needs: review label Aug 9, 2018

0xc0170 previously requested changes Aug 10, 2018

View reviewed changes

ithinuel reviewed Aug 13, 2018

View reviewed changes

OPpuolitaival force-pushed the icetea branch 3 times, most recently from 1d2ea0a to a3567f2 Compare August 17, 2018 09:50

jupe reviewed Aug 18, 2018

View reviewed changes

jupe reviewed Aug 20, 2018

View reviewed changes

0xc0170 previously requested changes Aug 20, 2018

View reviewed changes

cmonr added risk: G risk: A release-version: 5.10.0-rc1 and removed risk: G labels Aug 23, 2018

0xc0170 requested a review from a team August 24, 2018 12:22

0xc0170 requested review from a team and donatieng August 24, 2018 12:26

OPpuolitaival force-pushed the icetea branch from 9711ba2 to eff4dcf Compare August 27, 2018 08:53

bridadan reviewed Aug 27, 2018

View reviewed changes

donatieng approved these changes Aug 28, 2018

View reviewed changes

OPpuolitaival force-pushed the icetea branch from 2648461 to df72753 Compare August 29, 2018 13:48

cmonr added risk: R and removed risk: A labels Aug 29, 2018

cmonr removed the needs: work label Aug 31, 2018

theotherjimmy suggested changes Aug 31, 2018

View reviewed changes

Update icetea

6609faa

theotherjimmy approved these changes Aug 31, 2018

View reviewed changes

cmonr added risk: A and removed risk: R labels Aug 31, 2018

cmonr added needs: work risk: R and removed risk: A needs: CI labels Sep 1, 2018

cmonr added risk: A needs: CI and removed risk: R needs: work labels Sep 1, 2018

cmonr added risk: G and removed risk: A labels Sep 2, 2018

cmonr added ready for merge and removed needs: CI labels Sep 2, 2018

0xc0170 merged commit 1eab072 into ARMmbed:master Sep 2, 2018

0xc0170 removed the ready for merge label Sep 2, 2018


		Now that the interface has been selected you can run the icetea tests from the mbed-os root on your command line by

		`>mbed test -m <target> -t <toolchain> --icetea`


		### Structure

		mbed-os has a folder called TEST_APPS that contains everything related to IceTea -testing.


		#### Prerequisities

		You should have IceTea and forked mbed-cli that supports IceTea, installed.

		@@ -0,0 +1,2 @@
		test/*
		source/ns_list_internal/*

		@@ -0,0 +1 @@
		test_suite.json

Icetea support #7745

Icetea support #7745

Conversation

OPpuolitaival commented Aug 9, 2018

Description

Pull request type

0xc0170 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OPpuolitaival commented Aug 10, 2018

ithinuel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 commented Aug 24, 2018

bridadan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jupe Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 commented Aug 29, 2018

OPpuolitaival commented Aug 29, 2018

cmonr commented Aug 30, 2018

jarlamsa commented Aug 30, 2018

theotherjimmy left a comment

Choose a reason for hiding this comment

theotherjimmy left a comment

Choose a reason for hiding this comment

theotherjimmy commented Aug 31, 2018

cmonr commented Aug 31, 2018

mbed-ci commented Sep 1, 2018

jupe Aug 28, 2018 •

edited

Loading