Unittest regression fixes #526

stinos · 2022-09-06T11:57:52Z

I finally came around to getting my micropython-lib fork in sync, turns out it broke all my tests :) Strange no-one noticed this.

andrewleech · 2022-09-06T22:55:25Z

Thanks for the updates - when I originally tried to run "all tests in the repo" with the discover updates there were so many failures from a mixture of:

dependency hell
non-unittest based test scripts
tests that have clearly been broken for a long time
I basically stopped bothering trying to work out which tests should/shouldn't pass and made it work with the test usages I had / could find locally.

I'm curious about the first commit here:

Fix regression introduced by a7b2f63 which broke the typical case of
running a test script which has

if name == "main":
unittest.main()

This usage seems to work fine for me as-is on current master?

STEP: ~/micropython-lib/unix-ffi/unittest $ micropython ./test_unittest.py 
test_NotChangedByOtherTest (test_unittest_isolated.TestUnittestIsolated) ... ok

----------------------------------------------------------------------
Ran 1 tests

OK
testFail (test_unittest.TestUnittestAssertions) ... ok
testEqual (test_unittest.TestUnittestAssertions) ... ok
test_AlmostEqual (test_unittest.TestUnittestAssertions) ... ok
test_AlmostEqualWithDelta (test_unittest.TestUnittestAssertions) ... ok
testNotEqual (test_unittest.TestUnittestAssertions) ... ok
testIs (test_unittest.TestUnittestAssertions) ... ok
testIsNot (test_unittest.TestUnittestAssertions) ... ok
testIsNone (test_unittest.TestUnittestAssertions) ... ok
testIsNotNone (test_unittest.TestUnittestAssertions) ... ok
testTrue (test_unittest.TestUnittestAssertions) ... ok
testFalse (test_unittest.TestUnittestAssertions) ... ok
testIn (test_unittest.TestUnittestAssertions) ... ok
testIsInstance (test_unittest.TestUnittestAssertions) ... ok
testRaises (test_unittest.TestUnittestAssertions) ... ok
testSkip (test_unittest.TestUnittestAssertions) ... skipped: test of skipping
testAssert (test_unittest.TestUnittestAssertions) ... ok
testExpectedFailure (test_unittest.TestUnittestAssertions) ... ok
testExpectedFailureNot (test_unittest.TestUnittestAssertions) ... ok
test_NotChangedByOtherTest (test_unittest.TestUnittestAssertions) ... FAIL
test_subtest_even (test_unittest.TestUnittestAssertions) ... ok
testSetUpTearDownClass_1 (test_unittest.TestUnittestSetup) ... ok
testSetUpTearDownClass_2 (test_unittest.TestUnittestSetup) ... ok

======================================================================
FAIL: test_NotChangedByOtherTest <class 'TestUnittestAssertions'>
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/anl/micropython-lib/unix-ffi/unittest/unittest.py", line 393, in run_one
  File "<string>", line 1, in <module>
  File "./test_unittest.py", line 148, in test_NotChangedByOtherTest
AssertionError: 

----------------------------------------------------------------------
Ran 22 tests

FAILED (failures=1, errors=0)

Can you suggest a reproduction of the issue you ran into?
Your change looks clean, I'm just not following the usage of it.

andrewleech · 2022-09-06T23:00:41Z

unix-ffi/unittest/unittest.py

        if isinstance(c, object) and isinstance(c, type) and issubclass(c, TestCase):
            yield c
-        elif tn.startswith("test_") and callable(c):
+        elif tn.startswith("test") and callable(c):


Sorry for breaking some tests with this change... I don't remember clearly now but I think I found some existing test scripts in the repo where there were testFoo functions being run that were not intended to be unittest functions, so thought the _ made more sense. I believe this is non-standard functionality that cpython doesn't support anyway (finding & running bare test* functions)?

I believe this is non-standard functionality that cpython doesn't support anyway (finding & running bare test* functions)?

I think so yes, at least I didn't find anything which does that. I'd personally refrain from doing that because it's non-standard, and because if you have a file like that then decide it runs under CPython as well you might think its tests are all fine but they are just not being ran. CPython does support a single runTest function though like

class Test(unittest.TestCase): @staticmethod def runTest(): test = unittest.TestCase() test.assertEqual(1, 1)

But anyway test is the standard prefix; e.g. By default these are the method names beginning with test and testMethodPrefix = 'test' in unittest/loader.py.

andrewleech · 2022-09-06T23:04:03Z

unix-ffi/unittest/unittest.py

        result = discover(runner)
+    # If there's one argument assume it's the path to the script which called main(), so run its tests.
+    elif argn == 1:
+        result = run_module(runner, module, split_path(sys.argv[0])[0], None)


split_path(sys.argv[0])
This is to split out just the filename isn't it... I would think this will fail if you're running a unittest from a different folder?

Yeah that's basically os.path.dirname . Not sure why that would fail, the point of the path argument is to add the script directory to sys.path right? So python /path/to/test.py is going to add path/to. I just realize this is not actually needed though, the directory of the executing script gets added to sys.path anyway. Moreover, as the comment says this is an assumption but thinking about it: this is just wrong and I'm going to remove it. If anything argv[0] could be used as prog name for argparse but that's it.

stinos · 2022-09-07T09:41:24Z

Can you suggest a reproduction of the issue you ran into?

The output from your test actually shows the problem: it runs not just test_unittest, but both test_unittest and test_unittest_isolation, because it's running test discovery in the current directory. E.g. running micropython ./unittest.py will also run both tests (instead of nothing). Whereas going one directory up and running micropython ./unittest/test_unittest.py will discover all tests in unix_ffi. Or still another way: create a file

import unittest
import test_unittest

unittest.main(test_unittest)

in the unittest directory and it will simply ignore the argument to main() and instead run all tests in the directory.

Btw any idea why test_NotChangedByOtherTest fails on your machine?

Also: ping for my other comment

Fix regression introduced by a7b2f63 which broke the typical case of running a test script which has if __name__ == "__main__": unittest.main()

Fix regression introduced by 9d9ca3d.

CPython's unittest.main() function uses a different argument order than what we have now, so to avoid future problems with unclear error messages should we add arguments in the future, force callers to specify exact arguments.

stinos · 2022-09-07T10:33:32Z

I went over the code again and fixed some things:

basically if main is called with a module as argument it shouldn't look at argv at all, just run the module, so make it like that
passing a module to run_module would fail because the module was passed to TestSuit but that expects a string
force using keywords with main (not sure if that one is really needed)

Note the code still has the equivalent of if len(sys.argv) == 0: discover() because the original had that. but I'm not sure what it would mean for sys.argv to be empty?

andrewleech · 2022-09-07T11:05:00Z

The output from your test actually shows the problem: it runs not just test_unittest, but both test_unittest and test_unittest_isolation, because it's running test discovery in the current directory.

Ah I see now. That'll be why I hadn't noticed the bug - at first glance it looks like it's still working. But it's not doing the right thing certainly, and pulls in the dependency when it wasn't intended to. Nice fix, thanks!

andrewleech · 2022-09-07T11:06:47Z

Btw any idea why test_NotChangedByOtherTest fails on your machine?

That I'm not sure about, didn't get a chance to investigate when I ran that this morning.

andrewleech · 2022-09-07T11:13:06Z

unix-ffi/unittest/unittest.py

        runner = TestRunner()

-    if len(sys.argv) <= 1:
-        result = discover(runner)


Note the code still has the equivalent of if len(sys.argv) == 0: discover() because the original had that. but I'm not sure what it would mean for sys.argv to be empty?

Do you mean this original? This was intended to catch the len==1 mostly, main case of this is micropython -m unittest where sys.argv == ['unittest'].

It's true I don't know if there's ever a case where len(sys.argv) == 0 unless you're manipulating sys.argv manually in a wrapper script, in which case all bets are off really. Not sure if I had a specific reason to make this <= 1 rather than just == 1 other than maybe not wanting to leave an undefined case statement.

Do you mean this original?

Yes exactly.

But without the check it would be unconditionally indexing sys.argv which is also not very nice so I'll just leave it as-is.

These must be class methods decorated, and tearDownClass must be called after runTest() as well.

jimmo · 2022-09-09T15:36:29Z

Thanks @stinos . I think all of this should be addressed by #527 (comment)

Please let me know if not.

dpgeorge · 2022-09-13T00:10:10Z

Should be fixed in 796a598

stinos force-pushed the unittest-regression-fixes branch 2 times, most recently from 7f36020 to 705f32f Compare September 6, 2022 14:49

andrewleech reviewed Sep 6, 2022

View reviewed changes

stinos added 4 commits September 7, 2022 12:07

unif-ffi/unittest: Reinstate running of module passed to main().

021dc04

Fix regression introduced by a7b2f63 which broke the typical case of running a test script which has if __name__ == "__main__": unittest.main()

unif-ffi/unittest: Reinstate correct test function name prefix.

90f140a

Fix regression introduced by 9d9ca3d.

unif-ffi/unittest: Make path handling compatible with backslashes.

5fe2291

unif-ffi/unittest: Force keyword arguments for main().

094f3ef

CPython's unittest.main() function uses a different argument order than what we have now, so to avoid future problems with unclear error messages should we add arguments in the future, force callers to specify exact arguments.

stinos force-pushed the unittest-regression-fixes branch from 705f32f to 094f3ef Compare September 7, 2022 10:33

andrewleech reviewed Sep 7, 2022

View reviewed changes

unif-ffi/unittest: Make setup/tearDownClass CPython-compatible.

5d349c4

These must be class methods decorated, and tearDownClass must be called after runTest() as well.

jimmo mentioned this pull request Sep 9, 2022

python-stdlib/unittest: Move back from unix-ffi. #527

Closed

dpgeorge closed this Sep 13, 2022

stinos deleted the unittest-regression-fixes branch September 13, 2022 09:29

Unittest regression fixes #526

Unittest regression fixes #526

Uh oh!

Conversation

stinos commented Sep 6, 2022

Uh oh!

andrewleech commented Sep 6, 2022

Uh oh!

andrewleech Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

stinos Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

andrewleech Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

stinos Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

stinos commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stinos commented Sep 7, 2022

Uh oh!

andrewleech commented Sep 7, 2022

Uh oh!

andrewleech commented Sep 7, 2022

Uh oh!

andrewleech Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

stinos Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

jimmo commented Sep 9, 2022

Uh oh!

dpgeorge commented Sep 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stinos commented Sep 7, 2022 •

edited

Loading