tests/periph_i2c: Add automated tests by MrKevinWeiss · Pull Request #9409 · RIOT-OS/RIOT

MrKevinWeiss · 2018-06-25T11:46:31Z

Contribution description

This is all the components needed to run automated i2c tests. It is not great but so far it catches quite a few issues.

Issues/PRs references

#6577

tcschmidt · 2018-06-28T11:43:02Z

ping @smlng

smlng · 2018-06-28T18:13:47Z

tests/periph_i2c/main.c

-    if (argc < 5) {
-        INVALID_ARGS;
-        printf("Usage:\n%s ADDR REG FLAG BYTE0 [BYTE1 ...]\n", argv[0]);
+    dev = _check_param(argc, argv, 5, 5 + BUFSIZE,


shouldn't this _check_param(argc, argv, 6, (5 + BUFSIZE), because the 6 param contains the first byte, ie. the minimum. At least the help text suggests that the first byte is not optional, or not?

ah no it should be _check_param(argc, argv, 5, (5 + BUFSIZE -1), bc the last param is already the first byte hence buffer must be one less, right?

smlng · 2018-06-28T18:20:31Z

tests/periph_i2c/main.c

-    if (argc < 4) {
-        INVALID_ARGS;
-        printf("Usage:\n%s: ADDR FLAG BYTE0 [BYTE1 [BYTE_n [...]]]\n", argv[0]);
+    dev = _check_param(argc, argv, 4, 4 + BUFSIZE,


same here (see below) should be _check_param(argc, argv, 4, (4 + BUFSIZE -1,)

smlng · 2018-06-28T18:22:27Z

tests/periph_i2c/tests/base_if.py

+import serial.tools.list_ports
+
+
+class IfParams:


as discussed, use dictionary instead

smlng · 2018-06-28T18:23:32Z

tests/periph_i2c/tests/base_if.py

+        self.connect(port, baud)
+
+    def get_port(self):
+        if (self.__dev.isOpen()):


here (and below) .isOpen() is deprecated according to documentation, use .is_open instead

smlng · 2018-06-28T18:26:00Z

tests/periph_i2c/tests/base_if.py

+            self.connect(port)
+            ret = fxn().data
+            try:
+                logging.debug("ID rx: %s" % ret)


the new python way for string formatting would look like this:

logging.debug("ID rx: {}".format(ret))

simply replace every %s or %d and such by {},
that way you don't need to care about variable types (which is not very pythonic anyway)

smlng · 2018-06-28T18:34:12Z

tests/periph_i2c/tests/periph_i2c_if.py

+
+    def write_bytes(self, dev=__dev, addr=__addr, data=__data, flag=None):
+        str = ''
+        for val in data:


there is a short hand for that:

str = ' '.join(str(x) for x in data)

which you could also place directly into the commands below

smlng · 2018-06-28T18:35:20Z

tests/periph_i2c/tests/test.py

+        if (isinstance(val1, str) and isinstance(val2, str)):
+            if (val1.lower() == val2.lower()):
+                res = "PASS"
+        elif (isinstance(val1, list)):


you assume val2 is a list, too?

Yup but why check, if it isn't it will just get caught in the exception. Also this is a throwaway test, only as an example.

smlng · 2018-06-28T18:35:42Z

tests/periph_i2c/tests/test.py

+    return [res, val1, val2]
+
+
+class TestParam:


might also wanna use dictionary here?

Yup but I don't want to spend more time on this.

smlng · 2018-06-28T18:36:21Z

tests/periph_i2c/tests/test.py

+def write_test():
+    cmd_log = list()
+    t = Test('write test', 'Tests the write functionality for default i2c bus \
+(0) with the bpt', cmd_log)


indent please

Then the output includes the indents I think...

smlng · 2018-06-28T18:37:15Z

tests/periph_i2c/tests/test.py

+
+def print_full_result(test):
+    print('===================================================================\
+=============')


minor, but this looks a bit ugly - can't we live we no line break or less ==== to fit 80 chars?

Again throw away so it doesn't matter.

smlng · 2018-06-29T06:30:46Z

@MrKevinWeiss my comment on the python stuff are more informational, hence non blocking. But please have a look at the C stuff specifically the numbers for the argv checker - might be a possible buffer overflow?

MrKevinWeiss · 2018-07-02T08:05:44Z

I disagree with the Travis errors, the training white-spaces are a feature of the .md (so that it can have a newline) and the duplicated if else are not touched by me (as well as serve an actual purpose if certain flags are set).

aabadie · 2018-07-02T12:29:45Z

tests/periph_i2c/tests/README.md

@@ -0,0 +1,15 @@
+How To Run
+==========
+Connect the DUT I2C0 to the BPT.  Information for BPT setup can be found [here](https://github.com/MrKevinWeiss/Testing)


DUT and BPT acronyms should be described first here.

aabadie · 2018-07-02T12:46:50Z

the training white-spaces are a feature of the .md

Indeed, but I think (personal preference) that it's sometime better to add an extra newline in this case. If you strongly disagree, you can try to patch the trailing white spaces checker script to avoid this particular case in .md files.

aabadie

It would make the python scripts more readable with some comments or doctrings added. Class names could be made more explicit (What is DDIf ?).

Long lines in READMEs could be split as well (to fit in 80 characters line length).

Prefixing constants with double underscore in Python doesn't seem very common to me, (__BPT_ADDR, etc). Same for class constant attributes, do we real want to have name mangling ?
See PEP8 for reference.

In the tests/README.md, there's an 'How to run' section that redirect to the BPT project, but none of them explains precisely how (what) to run.
As a user/tester, the information I'm looking for is: how to wire my board to the BPT (well explained by the BPT project, what to run (what command and from where exactly). For the latter, the BPT project should point to the corresponding RIOT test folder.

cladmi · 2018-07-05T12:33:56Z

As we did some python reviews IRL yesterday and found that the API would still need some adapting, I will add the WIP label, this way you can rebase/squash if you want to until it stabilize.

MrKevinWeiss · 2018-07-05T12:59:15Z

Thanks, I will separate out the connection soon...

MrKevinWeiss · 2018-07-09T09:13:03Z

@smlng Ready for review then I will squash and rebase.

Changed shell to reflect the api very closely. This allows full access to each function for unit testing.

Add automated script to test devices against a known testers. This will make it easier to run tests instead of manual testing. This is something that works for now but will be better integrated later.

smlng · 2018-07-10T11:08:25Z

@dylad if you agree, we should get this merged swiftly. Besides adapting the test to the new API it also adds an automated test script which is very helpful.

dylad · 2018-07-10T11:11:05Z

@smlng If this PR is mature enough feels free to ACK/merge

MrKevinWeiss · 2018-07-10T13:54:48Z

@smlng what a tempting big green button... Don't you just want to see what happens when you press it?

…/autotester tests/periph_i2c: Add automated tests

…ster tests/periph_i2c: Add automated tests

MrKevinWeiss added Area: tests Area: tests and testing framework Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Jun 25, 2018

MrKevinWeiss self-assigned this Jun 25, 2018

MrKevinWeiss requested a review from smlng June 25, 2018 11:46

This was referenced Jun 25, 2018

tests/periph_i2c: Add test to run with bpt and the py if #9312

Closed

tests/periph_i2c: python interface for shell #9311

Closed

smlng added the TF: I2C Marks issues and PRs related to the work of the I²C rework task force label Jun 28, 2018

smlng suggested changes Jun 28, 2018

View reviewed changes

haukepetersen mentioned this pull request Jun 29, 2018

periph/i2c: tracking the remodeling #6577

Closed

47 tasks

MrKevinWeiss requested a review from cladmi June 29, 2018 15:25

aabadie reviewed Jul 2, 2018

View reviewed changes

aabadie mentioned this pull request Jul 2, 2018

cpu/stm32_common: various improvements in i2c_1 driver #9476

Merged

MrKevinWeiss force-pushed the pr/tests/i2c_periph/autotester branch from a9c4248 to d2d55fd Compare July 4, 2018 09:43

cladmi added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jul 5, 2018

MrKevinWeiss removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jul 9, 2018

tests/periph_i2c: Adapt to new i2c api

1efffcb

Changed shell to reflect the api very closely. This allows full access to each function for unit testing.

MrKevinWeiss force-pushed the pr/tests/i2c_periph/autotester branch from 9e9eb57 to bbed7c9 Compare July 10, 2018 09:03

tests/periph_i2c: Add automated testing script

3b6d0af

Add automated script to test devices against a known testers. This will make it easier to run tests instead of manual testing. This is something that works for now but will be better integrated later.

MrKevinWeiss force-pushed the pr/tests/i2c_periph/autotester branch from bbed7c9 to 3b6d0af Compare July 10, 2018 09:08

smlng approved these changes Jul 10, 2018

View reviewed changes

smlng merged commit ff372bf into RIOT-OS:new_i2c_if Jul 10, 2018

basilfx pushed a commit to basilfx/RIOT that referenced this pull request Jul 10, 2018

Merge pull request RIOT-OS#9409 from MrKevinWeiss/pr/tests/i2c_periph…

fece357

…/autotester tests/periph_i2c: Add automated tests

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

dylad pushed a commit to dylad/RIOT that referenced this pull request Jul 10, 2018

Merge pull request RIOT-OS#9409 from MrKevinWeiss/pr/tests/i2c_periph…

d95740b

…/autotester tests/periph_i2c: Add automated tests

MrKevinWeiss deleted the pr/tests/i2c_periph/autotester branch July 11, 2018 08:03

dylad pushed a commit that referenced this pull request Jul 11, 2018

Merge pull request #9409 from MrKevinWeiss/pr/tests/i2c_periph/autote…

a96efc8

…ster tests/periph_i2c: Add automated tests

smlng mentioned this pull request Aug 16, 2018

test: move periph interface lib to pythonlibs path #9786

Closed

Conversation

MrKevinWeiss commented Jun 25, 2018

Contribution description

Issues/PRs references

Uh oh!

tcschmidt commented Jun 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smlng commented Jun 29, 2018

Uh oh!

MrKevinWeiss commented Jul 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented Jul 2, 2018

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrKevinWeiss commented Jul 5, 2018

Uh oh!

MrKevinWeiss commented Jul 9, 2018

Uh oh!

smlng commented Jul 10, 2018

Uh oh!

dylad commented Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrKevinWeiss commented Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cladmi commented Jul 5, 2018 •

edited

Loading

dylad commented Jul 10, 2018 •

edited

Loading