[riot-notifications] [RIOT-OS/RIOT] tests/riotboot: make automatic script work with cc2538-bsl (#15446)

[Alexandre Abadie]

@aabadie requested changes on this pull request.

I'm still not satisfied by the new version but it's already better. I would like to avoid to put too much code in the `__main__` block and try to put the logic in a function. See my proposals below.

> @@ -54,27 +54,45 @@ def assert_check_slot(child, slotnum, version):
     child.expect_exact('>')
 
 
-def testfunc(child):
-    # Ask for current slot, should be 0 or 1
-    child.expect_exact('>')
-    child.sendline("curslotnr")
-    child.expect(r"Current slot=([0-1])")
-    slotnum = int(child.match.group(1))
+class riotbootDevice():

```suggestion
class RiotbootDevice():
```
(https://www.python.org/dev/peps/pep-0008/#class-names)

> +        self.ret = 0
+        self.ret |= run(self._get_current)

Not exactly what I had in mind. The class attributes should be `current_slot` and `current_app_ver` and could be computed by a dedicated method (`get_current_slot_and_app_ver`).
The call to run should not be done in the init but rather explicitly outside:

```python
dev = RiotbootDevice()
ret = run(dev->get_current_slot_and_app_ver)
if ret != 0:
    sys.exit(ret)

[...]
```

> +    # This test requires flashing slot binaries. Some flashers like
+    # cc2538-bsl need to attach to the serial terminal in order to be
+    # able to flash. Since `run(testfunc)` spawns a terminal a different
+    # `run(testfunc)` is called after every flash in order to detach and
+    # re-attach the terminal between flashes
+    dev = riotbootDevice()
+    # flash to both slots and verify
+    for version in [dev.app_ver + 1, dev.app_ver + 2]:
+        # update the stored app_ver and slot_num
+        dev.slot_num = dev.slot_num ^ 1
+        dev.app_ver = version
+        # flash
+        flash_slot(dev.slot_num, dev.app_ver)
+        # verify its running from the correct slot with the right version
+        dev.ret |= run(dev.test_current)
+
+    sys.exit(dev.ret)

```suggestion
    sys.exit(testfunc())
```

And testfunc would be like (I removed the comments on purpose so the snippet is smaller, but I hope you'll get the idea):

```python
def testfunc():
    dev = RiotbootDevice()
    ret = run(dev->get_current_slot_and_app_ver)
    if ret != 0:
        return ret

    for version in [dev.app_ver + 1, dev.app_ver + 2]:
        dev.slot_num = dev.slot_num ^ 1
        dev.app_ver = version
        dev.flash_current_slot()
        ret = run(dev.check_current_slot_and_app_ver)
        if ret != 0:
            return ret
    return 0
```

Doing like this also has the benefit to fail fast if something goes wrong. And `flash_slot` (renamed as `flash_current_slot`) would be a method of `RiotbootDevice` and I would rename `test_current` as `check_current_slot_and_app_ver` which is a more explicit name.

>  
-    print("TEST PASSED")
+    def test_current(self, child):
+        # Verify that running slot_num and app_ver match the stored ones
+        assert_check_slot(child, self.slot_num, self.app_ver)

Instead of calling an external function, I would rather put its content here.

>  
-    print("TEST PASSED")
+    def test_current(self, child):

```suggestion
    def check_current_slot_and_app_ver(self, child):
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/15446#pullrequestreview-533202236
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20201118/0ea3abfe/attachment-0001.htm>