Skip to content

Unittest regression fixes #526

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Unittest regression fixes #526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
021dc04
unif-ffi/unittest: Reinstate running of module passed to main().
stinos Sep 7, 2022
90f140a
unif-ffi/unittest: Reinstate correct test function name prefix.
stinos Sep 6, 2022
5fe2291
unif-ffi/unittest: Make path handling compatible with backslashes.
stinos Sep 6, 2022
094f3ef
unif-ffi/unittest: Force keyword arguments for main().
stinos Sep 7, 2022
5d349c4
unif-ffi/unittest: Make setup/tearDownClass CPython-compatible.
stinos Sep 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions unix-ffi/unittest/test_unittest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,10 +160,14 @@ def test_subtest_even(self):
class TestUnittestSetup(unittest.TestCase):
class_setup_var = 0

def setUpClass(self):
@classmethod
def setUpClass(cls):
assert cls is TestUnittestSetup
TestUnittestSetup.class_setup_var += 1

def tearDownClass(self):
@classmethod
def tearDownClass(cls):
assert cls is TestUnittestSetup
# Not sure how to actually test this, but we can check (in the test case below)
# that it hasn't been run already at least.
TestUnittestSetup.class_setup_var = -1
Expand Down
101 changes: 57 additions & 44 deletions unix-ffi/unittest/unittest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,23 +418,23 @@ def run_one(test_function):

set_up_class()

if hasattr(o, "runTest"):
name = str(o)
run_one(o.runTest)
return

for name in dir(o):
if name.startswith("test"):
m = getattr(o, name)
if not callable(m):
continue
run_one(m)

if callable(o):
name = o.__name__
run_one(o)

tear_down_class()
try:
if hasattr(o, "runTest"):
name = str(o)
run_one(o.runTest)
else:
for name in dir(o):
if name.startswith("test"):
m = getattr(o, name)
if not callable(m):
continue
run_one(m)

if callable(o):
name = o.__name__
run_one(o)
finally:
tear_down_class()

return exceptions

Expand All @@ -444,11 +444,11 @@ def _test_cases(mod):
c = getattr(mod, tn)
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):
Copy link
Contributor

@andrewleech andrewleech Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Author

@stinos stinos Sep 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

yield c


def run_module(runner, module, path, top):
def run_module(runner, module, *extra_paths):
if not module:
raise ValueError("Empty module name")

Expand All @@ -457,13 +457,12 @@ def run_module(runner, module, path, top):
sys.modules.update(__modules__)

sys_path_initial = sys.path[:]
# Add script dir and top dir to import path
sys.path.insert(0, str(path))
if top:
sys.path.insert(1, top)
for path in (path for path in reversed(extra_paths) if path):
sys.path.insert(0, str(path))

try:
suite = TestSuite(module)
m = __import__(module) if isinstance(module, str) else module
suite = TestSuite(m.__name__)
for c in _test_cases(m):
suite.addTest(c)
result = runner.run(suite)
Expand All @@ -481,7 +480,16 @@ def discover(runner: TestRunner):
return discover(runner=runner)


def main(module="__main__", testRunner=None):
def split_path(path):
path = path.replace("\\", "/")
if "/" in path:
return path.rsplit("/", 1)
else:
return ".", path


def main(module="__main__", **kwargs):
testRunner = kwargs.get("testRunner", None)
if testRunner:
if isinstance(testRunner, type):
runner = testRunner()
Expand All @@ -490,29 +498,34 @@ def main(module="__main__", testRunner=None):
else:
runner = TestRunner()

if len(sys.argv) <= 1:
result = discover(runner)
Copy link
Contributor

@andrewleech andrewleech Sep 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@stinos stinos Sep 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

elif sys.argv[0].split(".")[0] == "unittest" and sys.argv[1] == "discover":
result = discover(runner)
if module is not None:
result = run_module(runner, module)
else:
for test_spec in sys.argv[1:]:
try:
uos.stat(test_spec)
# test_spec is a local file, run it directly
if "/" in test_spec:
path, fname = test_spec.rsplit("/", 1)
else:
path, fname = ".", test_spec
modname = fname.rsplit(".", 1)[0]
result = run_module(runner, modname, path, None)

except OSError:
# Not a file, treat as import name
result = run_module(runner, test_spec, ".", None)
argn = len(sys.argv)
# See https://docs.python.org/3/library/unittest.html#test-discovery:
# python -m unittest is the equivalent of python -m unittest discover.
if not argn or (
sys.argv[0].split(".")[0] == "unittest"
and (argn == 1 or (argn >= 1 and sys.argv[1] == "discover"))
):
result = discover(runner)
# Remaining arguments should be test specifiers.
else:
for test_spec in sys.argv[1:]:
try:
uos.stat(test_spec)
# test_spec is a local file, run it directly
path, fname = split_path(test_spec)
modname = fname.rsplit(".", 1)[0]
result = run_module(runner, modname, path)

except OSError:
# Not a file, treat as import name
result = run_module(runner, test_spec, ".")

# Terminate with non zero return code in case of failures
sys.exit(result.failuresNum or result.errorsNum)


if __name__ == "__main__":
main()
main(None)