| Message ID | 20210928221111.1162779-1-dlatypov@google.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: tool: misc fixes (unused vars, imports, leaked files) | expand |
On Wed, Sep 29, 2021 at 6:11 AM Daniel Latypov <dlatypov@google.com> wrote: > > Drop some variables in unit tests that were unused and/or add assertions > based on them. > > For ExitStack, it was imported, but the `es` variable wasn't used so it > didn't do anything, and we were leaking the file objects. > Refactor it to just use nested `with` statements to properly close them. > > And drop the direct use of .close() on file objects in the kunit tool > unit test, as these can be leaked if test assertions fail. To clarify for a python novice: this is referring to using "with" so that the file isn't leaked if the assertion fails, rather than suggesting that leaks are okay for failing tests, right? > Signed-off-by: Daniel Latypov <dlatypov@google.com> > --- These all seem sensible to me. Thanks for cleaning this up! Reviewed-by: David Gow <davidgow@google.com> -- David
On Tue, Sep 28, 2021 at 5:34 PM 'David Gow' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > On Wed, Sep 29, 2021 at 6:11 AM Daniel Latypov <dlatypov@google.com> wrote: > > > > Drop some variables in unit tests that were unused and/or add assertions > > based on them. > > > > For ExitStack, it was imported, but the `es` variable wasn't used so it > > didn't do anything, and we were leaking the file objects. > > Refactor it to just use nested `with` statements to properly close them. > > > > And drop the direct use of .close() on file objects in the kunit tool > > unit test, as these can be leaked if test assertions fail. > > To clarify for a python novice: this is referring to using "with" so > that the file isn't leaked if the assertion fails, rather than > suggesting that leaks are okay for failing tests, right? Correct. Ah, I see how it can be misread now. But on that note, it's probably fine to leak the files as CPython should close the file object during GC. And tests failing should make those file objects go out of scope immediately. > > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > --- > > These all seem sensible to me. Thanks for cleaning this up! > > Reviewed-by: David Gow <davidgow@google.com> > > -- David > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/CABVgOS%3D0K78N%2BKMK3km5TKVDD9L8AMRpNCfvihCqU2h3U-oE-w%40mail.gmail.com.
On Tue, Sep 28, 2021 at 3:11 PM Daniel Latypov <dlatypov@google.com> wrote: > > Drop some variables in unit tests that were unused and/or add assertions > based on them. > > For ExitStack, it was imported, but the `es` variable wasn't used so it > didn't do anything, and we were leaking the file objects. > Refactor it to just use nested `with` statements to properly close them. > > And drop the direct use of .close() on file objects in the kunit tool > unit test, as these can be leaked if test assertions fail. > > Signed-off-by: Daniel Latypov <dlatypov@google.com> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 66f67af97971..1b2b7f06bb8c 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -18,7 +18,6 @@ from collections import namedtuple from enum import Enum, auto from typing import Iterable -import kunit_config import kunit_json import kunit_kernel import kunit_parser diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index 2c6f916ccbaf..1870e75ff153 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -14,10 +14,6 @@ import shutil import signal from typing import Iterator, Optional, Tuple -from contextlib import ExitStack - -from collections import namedtuple - import kunit_config import kunit_parser import qemu_config @@ -168,10 +164,10 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations): process.wait() kunit_parser.print_with_timestamp( 'Disabling broken configs to run KUnit tests...') - with ExitStack() as es: - config = open(get_kconfig_path(build_dir), 'a') - disable = open(BROKEN_ALLCONFIG_PATH, 'r').read() - config.write(disable) + + with open(get_kconfig_path(build_dir), 'a') as config: + with open(BROKEN_ALLCONFIG_PATH, 'r') as disable: + config.write(disable.read()) kunit_parser.print_with_timestamp( 'Starting Kernel with all configs takes a few minutes...') diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index 619c4554cbff..cad37a98e599 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -185,7 +185,7 @@ class KUnitParserTest(unittest.TestCase): kunit_parser.extract_tap_lines(file.readlines())) print_mock.assert_any_call(StrContains('could not parse test results!')) print_mock.stop() - file.close() + self.assertEqual(0, len(result.suites)) def test_crashed_test(self): crashed_log = test_data_path('test_is_test_passed-crash.log') @@ -197,24 +197,22 @@ class KUnitParserTest(unittest.TestCase): def test_skipped_test(self): skipped_log = test_data_path('test_skip_tests.log') - file = open(skipped_log) - result = kunit_parser.parse_run_tests(file.readlines()) + with open(skipped_log) as file: + result = kunit_parser.parse_run_tests(file.readlines()) # A skipped test does not fail the whole suite. self.assertEqual( kunit_parser.TestStatus.SUCCESS, result.status) - file.close() def test_skipped_all_tests(self): skipped_log = test_data_path('test_skip_all_tests.log') - file = open(skipped_log) - result = kunit_parser.parse_run_tests(file.readlines()) + with open(skipped_log) as file: + result = kunit_parser.parse_run_tests(file.readlines()) self.assertEqual( kunit_parser.TestStatus.SKIPPED, result.status) - file.close() def test_ignores_prefix_printk_time(self): @@ -283,13 +281,13 @@ class LinuxSourceTreeTest(unittest.TestCase): def test_valid_kunitconfig(self): with tempfile.NamedTemporaryFile('wt') as kunitconfig: - tree = kunit_kernel.LinuxSourceTree('', kunitconfig_path=kunitconfig.name) + kunit_kernel.LinuxSourceTree('', kunitconfig_path=kunitconfig.name) def test_dir_kunitconfig(self): with tempfile.TemporaryDirectory('') as dir: - with open(os.path.join(dir, '.kunitconfig'), 'w') as f: + with open(os.path.join(dir, '.kunitconfig'), 'w'): pass - tree = kunit_kernel.LinuxSourceTree('', kunitconfig_path=dir) + kunit_kernel.LinuxSourceTree('', kunitconfig_path=dir) # TODO: add more test cases.
Drop some variables in unit tests that were unused and/or add assertions based on them. For ExitStack, it was imported, but the `es` variable wasn't used so it didn't do anything, and we were leaking the file objects. Refactor it to just use nested `with` statements to properly close them. And drop the direct use of .close() on file objects in the kunit tool unit test, as these can be leaked if test assertions fail. Signed-off-by: Daniel Latypov <dlatypov@google.com> --- tools/testing/kunit/kunit.py | 1 - tools/testing/kunit/kunit_kernel.py | 12 ++++-------- tools/testing/kunit/kunit_tool_test.py | 18 ++++++++---------- 3 files changed, 12 insertions(+), 19 deletions(-) base-commit: 3b29021ddd10cfb6b2565c623595bd3b02036f33