<p></p>
<p><b>@aabadie</b> requested changes on this pull request.</p>

<p>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 <code>__main__</code> block and try to put the logic in a function. See my proposals below.</p><hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/15446#discussion_r525896289">tests/riotboot/tests/01-run.py</a>:</p>
<pre style='color:#555'>> @@ -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():
</pre>

⬇️ Suggested change
<pre style="color: #555">-class riotbootDevice():
+class RiotbootDevice():
</pre>

<p>(<a href="https://www.python.org/dev/peps/pep-0008/#class-names" rel="nofollow">https://www.python.org/dev/peps/pep-0008/#class-names</a>)</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/15446#discussion_r525899052">tests/riotboot/tests/01-run.py</a>:</p>
<pre style='color:#555'>> +        self.ret = 0
+        self.ret |= run(self._get_current)
</pre>
<p>Not exactly what I had in mind. The class attributes should be <code>current_slot</code> and <code>current_app_ver</code> and could be computed by a dedicated method (<code>get_current_slot_and_app_ver</code>).<br>
The call to run should not be done in the init but rather explicitly outside:</p>
<div class="highlight highlight-source-python"><pre><span class="pl-s1">dev</span> <span class="pl-c1">=</span> <span class="pl-v">RiotbootDevice</span>()
<span class="pl-s1">ret</span> <span class="pl-c1">=</span> <span class="pl-en">run</span>(<span class="pl-s1">dev</span><span class="pl-c1">-</span><span class="pl-c1">></span><span class="pl-s1">get_current_slot_and_app_ver</span>)
<span class="pl-k">if</span> <span class="pl-s1">ret</span> <span class="pl-c1">!=</span> <span class="pl-c1">0</span>:
    <span class="pl-s1">sys</span>.<span class="pl-en">exit</span>(<span class="pl-s1">ret</span>)

[...]</pre></div>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/15446#discussion_r526007727">tests/riotboot/tests/01-run.py</a>:</p>
<pre style='color:#555'>> +    # 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)
</pre>

⬇️ Suggested change
<pre style="color: #555">-    # 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)
+    sys.exit(testfunc())
</pre>

<p>And testfunc would be like (I removed the comments on purpose so the snippet is smaller, but I hope you'll get the idea):</p>
<div class="highlight highlight-source-python"><pre><span class="pl-k">def</span> <span class="pl-en">testfunc</span>():
    <span class="pl-s1">dev</span> <span class="pl-c1">=</span> <span class="pl-v">RiotbootDevice</span>()
    <span class="pl-s1">ret</span> <span class="pl-c1">=</span> <span class="pl-en">run</span>(<span class="pl-s1">dev</span><span class="pl-c1">-</span><span class="pl-c1">></span><span class="pl-s1">get_current_slot_and_app_ver</span>)
    <span class="pl-k">if</span> <span class="pl-s1">ret</span> <span class="pl-c1">!=</span> <span class="pl-c1">0</span>:
        <span class="pl-k">return</span> <span class="pl-s1">ret</span>

    <span class="pl-k">for</span> <span class="pl-s1">version</span> <span class="pl-c1">in</span> [<span class="pl-s1">dev</span>.<span class="pl-s1">app_ver</span> <span class="pl-c1">+</span> <span class="pl-c1">1</span>, <span class="pl-s1">dev</span>.<span class="pl-s1">app_ver</span> <span class="pl-c1">+</span> <span class="pl-c1">2</span>]:
        <span class="pl-s1">dev</span>.<span class="pl-s1">slot_num</span> <span class="pl-c1">=</span> <span class="pl-s1">dev</span>.<span class="pl-s1">slot_num</span> <span class="pl-c1">^</span> <span class="pl-c1">1</span>
        <span class="pl-s1">dev</span>.<span class="pl-s1">app_ver</span> <span class="pl-c1">=</span> <span class="pl-s1">version</span>
        <span class="pl-s1">dev</span>.<span class="pl-en">flash_current_slot</span>()
        <span class="pl-s1">ret</span> <span class="pl-c1">=</span> <span class="pl-en">run</span>(<span class="pl-s1">dev</span>.<span class="pl-s1">check_current_slot_and_app_ver</span>)
        <span class="pl-k">if</span> <span class="pl-s1">ret</span> <span class="pl-c1">!=</span> <span class="pl-c1">0</span>:
            <span class="pl-k">return</span> <span class="pl-s1">ret</span>
    <span class="pl-k">return</span> <span class="pl-c1">0</span></pre></div>
<p>Doing like this also has the benefit to fail fast if something goes wrong. And <code>flash_slot</code> (renamed as <code>flash_current_slot</code>) would be a method of <code>RiotbootDevice</code> and I would rename <code>test_current</code> as <code>check_current_slot_and_app_ver</code> which is a more explicit name.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/15446#discussion_r526009555">tests/riotboot/tests/01-run.py</a>:</p>
<pre style='color:#555'>>  
-    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)
</pre>
<p>Instead of calling an external function, I would rather put its content here.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/15446#discussion_r526009779">tests/riotboot/tests/01-run.py</a>:</p>
<pre style='color:#555'>>  
-    print("TEST PASSED")
+    def test_current(self, child):
</pre>

⬇️ Suggested change
<pre style="color: #555">-    def test_current(self, child):
+    def check_current_slot_and_app_ver(self, child):
</pre>


<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/RIOT-OS/RIOT/pull/15446#pullrequestreview-533202236">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABE7WYBECM2QMGEWDAFBQELSQOVNDANCNFSM4TW4Z6IQ">unsubscribe</a>.<img src="https://github.com/notifications/beacon/ABE7WYAPML53SOWUB4YGJX3SQOVNDA5CNFSM4TW4Z6I2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOD7EAKPA.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/RIOT-OS/RIOT/pull/15446#pullrequestreview-533202236",
"url": "https://github.com/RIOT-OS/RIOT/pull/15446#pullrequestreview-533202236",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>