New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 788648 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

[chromite] fix unittests to be compatible with mock 2.0.

Project Member Reported by deanliao@chromium.org, Nov 27 2017

Issue description

Because a typo was found in code review (see below), I figured out that  assert_not_called, assert_called, assert_called_once mock validator are not implemented in third_party/mock.py. And because it is a mock object, any calls is valid, so:
  m = PatchObject(...)
  ...
  m.assert_called()
  m.assert_not_called()
  can pass.

The code review: https://chromium-review.googlesource.com/c/chromiumos/chromite/+/765641/3/cros_bisect/chrome_on_cros_bisector_unittest.py#389
 
I plan to back-port the methods from https://github.com/testing-cabal/mock/blob/master/mock/mock.py
Cc: vapier@chromium.org
Backporting assert_not_called, assert_called, assert_called_once mock methods fails the following unittests:
See https://chromium-review.googlesource.com/c/chromiumos/chromite/+/789748
cbuildbot/stages/sync_stages_unittest PreCQLauncherStageTest.testLaunchTrybots
cbuildbot/stages/test_stages_unittest HWTestStageTest.testHandleBoardNotAvailabl
cros_bisect/git_bisector_unittest TestGitBisector.testSanityCheckGoodCommitNotExist
cros_bisect/git_bisector_unittest TestGitBisector.testSanityCheckSyncToHeadWorks


Mike suggests to drop mock 2.0.0 rather than just backport the three methods. Trying.
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/9284be310fea9f6788261c873d5feb1166fe5b39

commit 9284be310fea9f6788261c873d5feb1166fe5b39
Author: Dean Liao <deanliao@chromium.org>
Date: Thu Dec 14 06:33:28 2017

[chromite] fix unittests for mock call-assertion.

It is a preparation for mock-2.0.0 upgrade.

BUG= chromium:788648 
TEST=tryjob

Change-Id: I056067ab8473f440cd7a87020eea742b6e7b7b83
Reviewed-on: https://chromium-review.googlesource.com/822791
Commit-Ready: Shuo-Peng Liao <deanliao@chromium.org>
Tested-by: Shuo-Peng Liao <deanliao@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/9284be310fea9f6788261c873d5feb1166fe5b39/cbuildbot/stages/test_stages_unittest.py
[modify] https://crrev.com/9284be310fea9f6788261c873d5feb1166fe5b39/cbuildbot/stages/sync_stages_unittest.py

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/8bf12e3f5cb1e59e5922e4d7f77784aeaee13447

commit 8bf12e3f5cb1e59e5922e4d7f77784aeaee13447
Author: Dean Liao <deanliao@chromium.org>
Date: Thu Dec 14 12:22:37 2017

[chromite] Fix partial_mock's side_effect behavior.

Respect Python's mock object's side_effect definition:
side_effect: ... Alternatively side_effect can be an exception
class or instance. In this case the exception will be raised when
the mock is called.
https://docs.python.org/3/library/unittest.mock.html#the-mock-class

BUG= chromium:788648 
TEST=tryjob

Change-Id: I464851f808b051a0c2fc1dc6c3542114cda9dedd
Reviewed-on: https://chromium-review.googlesource.com/823745
Commit-Ready: Shuo-Peng Liao <deanliao@chromium.org>
Tested-by: Shuo-Peng Liao <deanliao@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/8bf12e3f5cb1e59e5922e4d7f77784aeaee13447/lib/partial_mock.py
[modify] https://crrev.com/8bf12e3f5cb1e59e5922e4d7f77784aeaee13447/lib/partial_mock_unittest.py

Project Member

Comment 5 by bugdroid1@chromium.org, Feb 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/861b07d864ddb47ef80df4a98deb809db5dbbb8c

commit 861b07d864ddb47ef80df4a98deb809db5dbbb8c
Author: Dean Liao <deanliao@chromium.org>
Date: Thu Feb 01 10:41:57 2018

[chromite] fix git_bisector bugs caught by mock 2.0.0 upgrade.

For testSanityCheckGoodCommitNotExist() and
testSanityCheckSyncToHeadWorks(), it has a bug that it falsely
claims that git_mock makes DoesCommitExistInRepo() returning
False. It was not caught because the current mock library it
uses is 1.0.1 (third_party/mock), which does not implement
assert_called(). And also for mocked object, any unseen method
invocation is okay, so the verification code:
sync_to_head_mock.assert_called() always
passes.

After fixing the above bug by making GitMock raises RunCommandError
exception (see AddRunGitResult() change), as GitMock now raises
error for non-zero returncode, it triggers another bug in
GitBisector.Git(). The original implementation does not pass
error_code_ok=True to git.RunGit() so that when git command
returns nonzero, it raises exception instead of returns
CommandResult with nonzero returncode, which breaks the logic like
UpdateCurrentCommit(), GetCommitTimestamp(), GitBisect().

GitMock.RunGit() also has change to reflect GitBisector.Git() fix.

BUG= chromium:788648 
TEST=manual
1. Remove third_party/mock.py* so that inside chroot, mock will
use version 2.0.0.
2. Without this CL, cros_bisect/git_bisector_unittest fails inside chroot.
3. With this CL, cros_bisect/git_bisector_unittest success inside chroot.

Change-Id: I25e8f5bc117af89e1866bdf79d7d2a0c0b7d139e
Reviewed-on: https://chromium-review.googlesource.com/826824
Commit-Ready: Shuo-Peng Liao <deanliao@chromium.org>
Tested-by: Shuo-Peng Liao <deanliao@chromium.org>
Reviewed-by: Chung-yih Wang <cywang@chromium.org>

[modify] https://crrev.com/861b07d864ddb47ef80df4a98deb809db5dbbb8c/cros_bisect/git_bisector.py
[modify] https://crrev.com/861b07d864ddb47ef80df4a98deb809db5dbbb8c/cros_bisect/git_bisector_unittest.py

Project Member

Comment 6 by bugdroid1@chromium.org, Feb 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/e5b0aca290c3a9bf5eee70e7148566501b93f584

commit e5b0aca290c3a9bf5eee70e7148566501b93f584
Author: Dean Liao <deanliao@chromium.org>
Date: Fri Feb 02 18:17:05 2018

[chromite] remove mock dependency for non-test code.

While I planned to remove third_party/mock to use default mock
in chroot, I found mock dependency in non-test code. This makes
chromite unable to create chroot if the host system does not
have mock module. For non-test code, it should not depend on
mock module so this commit is used to remove mock dependency.

For third_party/mock removal plan, it will be changed as:
1. Upgrade third_party/mock to 2.0.0 with patch to fix
   classmethod patching issue.
2. Remove third_party/mock after Python ToT accepts the patch.

BUG= chromium:788648 
TEST=tryjob

Change-Id: Ie5955a785da58849ca33deca132426eebe4da5c3
Reviewed-on: https://chromium-review.googlesource.com/882528
Commit-Ready: Shuo-Peng Liao <deanliao@chromium.org>
Tested-by: Shuo-Peng Liao <deanliao@chromium.org>
Reviewed-by: Chung-yih Wang <cywang@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/e5b0aca290c3a9bf5eee70e7148566501b93f584/scripts/cbuildbot.py
[modify] https://crrev.com/e5b0aca290c3a9bf5eee70e7148566501b93f584/lib/gs.py
[modify] https://crrev.com/e5b0aca290c3a9bf5eee70e7148566501b93f584/lib/patch.py

Status: Fixed (was: Started)
Summary: [chromite] fix unittests to be compatible with mock 2.0. (was: [chromite] assert_called / assert_not_called does not work (always pass))
The bug was to address useless statements:
mock_obj.assert_called()
mock_obj.assert_not_called()

It turned out that upgrading third_party/mock to 2.0.0, and the next step: removing third_party/mock to rely on mock module in host system, is the right direction. So the commits are for the preparation of using third_party/mock and  mock dependency removal of non-test code.

For third_party/mock removal, it encounters a mock 2.0.0 module's bug in Python issue tracker:
https://bugs.python.org/issue23078
mock 2.0.0 cannot patch classmethod / staticmethod of a class correctly

The removal plan is blocked by it.

Sign in to add a comment