[riot-notifications] [RIOT-OS/RIOT] Tests/pkg_fatfs_vfs: make test fail correctly for samr21-xpro (#11389)

MichelRottleuthner notifications at github.com
Thu Apr 18 14:43:01 CEST 2019


MichelRottleuthner requested changes on this pull request.

Looking at this test again reminds me why I dislike using pexpect for testing...
First: the changes in this PR may fix throwing an exception if there is something wrong - but with this PR it also breaks that the test, well, actually tests something ;)
As @miri64 pointed out already the loop that actually checks the error states needs to be added again.

To illustrate the thing regarding pexpect: the test outputs multiple results, one for each thing that is tested. This was intended to give more detailed feedback to the developer running the test. E.g. see which part is failing without modifying the test app.
Writing something like `child.expect(u"[^\n]*:\[OK\]\r\n")` somehow suggests that it checks for "blabla [OK]" and fails otherwise. What actually happens is that the expect call will not return on lines other than the expected one and continues to wait till a line with "..[OK]" is printed. So some spurious output between the OK lines wont break the test. And getting a lot of "... [FAILED]" with one "... [OK]" at the end would also count as as a successful test. IMO any unexpected output during the test - wherever it might come from - should make the test fail. And that is what I wanted to do in this test: catch whatever the test might produce and act upon it.

Regarding the reasoning to remove the exception I generally agree, but how do you correctly hand a return code up to testrunner?
Actually a lot of the other tests throw a timeout *exception* if the expected output never shows up.
If you look at the testrunner you will see that the return code of the testfunc is actually never checked for anything. So the only way to make the test fail (without touching testrunner) is to throw an exception.



-- 
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/11389#pullrequestreview-228261631
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190418/680813fb/attachment.html>


More information about the notifications mailing list