| Message ID | 20201202190824.1309398-1-dlatypov@google.com |
|---|---|
| State | Accepted |
| Commit | cfd607e43da4a20753744f134e201310262b827a |
| Headers | show |
| Series | [v2,1/4] kunit: tool: fix unit test cleanup handling | expand |
On Thu, Dec 3, 2020 at 3:09 AM Daniel Latypov <dlatypov@google.com> wrote: > > * Stop leaking file objects. > * Use self.addCleanup() to ensure we call cleanup functions even if > setUp() fails. > * use mock.patch.stopall instead of more error-prone manual approach > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > --- This patch hasn't changed since v1, right? It's still: Reviewed-by: David Gow <davidgow@google.com> Cheers, -- David
On Thu, Dec 3, 2020 at 3:09 AM Daniel Latypov <dlatypov@google.com> wrote: > > Use self.assertEqual/assertNotEqual() instead. > Besides being more appropriate in a unit test, it'll also give a better > error message by show the unexpected values. > > Also > * Delete redundant check of exception types. self.assertRaises does this. > * s/kall/call. There's no reason to name it this way. > * This is probably a misunderstanding from the docs which uses it > since `mock.call` is in scope as `call`. > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > --- Looks good, thanks! Reviewed-by: David Gow <davidgow@google.com> -- David
On Thu, Dec 3, 2020 at 3:09 AM Daniel Latypov <dlatypov@google.com> wrote: > > The use of manual open() and .close() calls seems to be an attempt to > keep the contents in scope. > But Python doesn't restrict variables like that, so we can introduce new > variables inside of a `with` and use them outside. > > Do so to make the code more Pythonic. > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > --- Reviewed-by: David Gow <davidgow@google.com> Cheers, -- David
On Wed, Dec 2, 2020 at 7:05 PM David Gow <davidgow@google.com> wrote: > > On Thu, Dec 3, 2020 at 3:09 AM Daniel Latypov <dlatypov@google.com> wrote: > > > > * Stop leaking file objects. > > * Use self.addCleanup() to ensure we call cleanup functions even if > > setUp() fails. > > * use mock.patch.stopall instead of more error-prone manual approach > > > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > --- > > This patch hasn't changed since v1, right? > > It's still: > Reviewed-by: David Gow <davidgow@google.com> Oops, yes. It's entirely unchanged. The only change to the entire series was a rebase + drop the second patch in favor of revamping the test_data_path() one. > > Cheers, > -- David
On Wed, Dec 2, 2020 at 11:09 AM Daniel Latypov <dlatypov@google.com> wrote: > > The use of manual open() and .close() calls seems to be an attempt to > keep the contents in scope. > But Python doesn't restrict variables like that, so we can introduce new > variables inside of a `with` and use them outside. > > Do so to make the code more Pythonic. > > Signed-off-by: Daniel Latypov <dlatypov@google.com> Tested-by: Brendan Higgins <brendanhiggins@google.com> Acked-by: Brendan Higgins <brendanhiggins@google.com>
On Wed, Dec 2, 2020 at 11:09 AM Daniel Latypov <dlatypov@google.com> wrote: > > Use self.assertEqual/assertNotEqual() instead. > Besides being more appropriate in a unit test, it'll also give a better > error message by show the unexpected values. > > Also > * Delete redundant check of exception types. self.assertRaises does this. > * s/kall/call. There's no reason to name it this way. > * This is probably a misunderstanding from the docs which uses it > since `mock.call` is in scope as `call`. > > Signed-off-by: Daniel Latypov <dlatypov@google.com> Tested-by: Brendan Higgins <brendanhiggins@google.com> Acked-by: Brendan Higgins <brendanhiggins@google.com>
On Wed, Dec 2, 2020 at 11:09 AM Daniel Latypov <dlatypov@google.com> wrote: > > * Stop leaking file objects. > * Use self.addCleanup() to ensure we call cleanup functions even if > setUp() fails. > * use mock.patch.stopall instead of more error-prone manual approach > > Signed-off-by: Daniel Latypov <dlatypov@google.com> Tested-by: Brendan Higgins <brendanhiggins@google.com> Acked-by: Brendan Higgins <brendanhiggins@google.com>
diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index 497ab51bc170..3fbe1acd531a 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -288,19 +288,17 @@ class StrContains(str): class KUnitMainTest(unittest.TestCase): def setUp(self): path = get_absolute_path('test_data/test_is_test_passed-all_passed.log') - file = open(path) - all_passed_log = file.readlines() - self.print_patch = mock.patch('builtins.print') - self.print_mock = self.print_patch.start() + with open(path) as file: + all_passed_log = file.readlines() + + self.print_mock = mock.patch('builtins.print').start() + self.addCleanup(mock.patch.stopall) + self.linux_source_mock = mock.Mock() self.linux_source_mock.build_reconfig = mock.Mock(return_value=True) self.linux_source_mock.build_um_kernel = mock.Mock(return_value=True) self.linux_source_mock.run_kernel = mock.Mock(return_value=all_passed_log) - def tearDown(self): - self.print_patch.stop() - pass - def test_config_passes_args_pass(self): kunit.main(['config', '--build_dir=.kunit'], self.linux_source_mock) assert self.linux_source_mock.build_reconfig.call_count == 1
* Stop leaking file objects. * Use self.addCleanup() to ensure we call cleanup functions even if setUp() fails. * use mock.patch.stopall instead of more error-prone manual approach Signed-off-by: Daniel Latypov <dlatypov@google.com> --- tools/testing/kunit/kunit_tool_test.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) base-commit: 509a15421674b9e1a3e1916939d0d0efd3e578da