New issue
Advanced search Search tips

Issue 641943 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Sep 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Refactor the mock in unit test for disk_mount_manager

Project Member Reported by yamaguchi@chromium.org, Aug 29 2016

Issue description

https://cs.chromium.org/chromium/src/chromeos/disks/disk_mount_manager_unittest.cc
Currently the unit test uses gmock. It's difficult to read in some parts.
Replace the mocking mechanism by gmock with a dedicated mock or fake class so as to improve the readability and maintainability of the test.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7bec88d891d568be4392e3e5d74d04b79ff4e667

commit 7bec88d891d568be4392e3e5d74d04b79ff4e667
Author: yamaguchi <yamaguchi@chromium.org>
Date: Tue Sep 13 06:10:37 2016

Add a mock class for DiskMountManagerObserver.
disk_mount_manager_unittest.cc will be rewritten using this class instead of gmock.

BUG= 641943 

Review-Url: https://codereview.chromium.org/2292473002
Cr-Commit-Position: refs/heads/master@{#418182}

[modify] https://crrev.com/7bec88d891d568be4392e3e5d74d04b79ff4e667/chromeos/disks/disk_mount_manager_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Sep 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dae37c463b67bc923d301e347177c7d5c874cce7

commit dae37c463b67bc923d301e347177c7d5c874cce7
Author: kjellander <kjellander@chromium.org>
Date: Tue Sep 13 11:10:44 2016

Revert of Add a mock class for DiskMountManagerObserver. disk_mount_manager_observer_unittests will be rewrit… (patchset #17 id:320001 of https://codereview.chromium.org/2292473002/ )

Reason for revert:
Breaks "Linux Chromium OS ASan LSan Tests (1)" like this:
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/16114

Snippet:
==24096==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 40 byte(s) in 1 object(s) allocated from:
    #0 0x55a84b in operator new(unsigned long) (/b/swarming/w/iroDkF6G/out/Release/chromeos_unittests+0x55a84b)
    #1 0x805c51 in (anonymous namespace)::DiskMountManagerTest::SetUp() chromeos/disks/disk_mount_manager_unittest.cc:428:9
    #2 0x2c3a936 in HandleExceptionsInMethodIfSupported<testing::Test, void> testing/gtest/src/gtest.cc:2458:12
    #3 0x2c3a936 in testing::Test::Run() testing/gtest/src/gtest.cc:2470
    #4 0x2c3cafb in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2656:11
    #5 0x2c3d8b6 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2774:28
    #6 0x2c518b6 in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4647:43
    #7 0x2c50f17 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2458:12
    #8 0x2c50f17 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4255
    #9 0x290b60b in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:46
    #10 0x290b60b in base::TestSuite::Run() base/test/test_suite.cc:246
    #11 0x2910983 in Run base/callback.h:64:12
    #12 0x2910983 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback<int (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&, int, int, bool, base::Callback<void (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) base/test/launcher/unit_test_launcher.cc:206
    #13 0x2910665 in base::LaunchUnitTests(int, char**, base::Callback<int (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) base/test/launcher/unit_test_launcher.cc:445:10
    #14 0x2909844 in main base/test/run_all_unittests.cc:12:10
    #15 0x7f3c903677ec in __libc_start_main /build/eglibc-oqps9y/eglibc-2.15/csu/libc-start.c:226

Original issue's description:
> Add a mock class for DiskMountManagerObserver.
> disk_mount_manager_unittest.cc will be rewritten using this class instead of gmock.
>
> BUG= 641943 
>
> Committed: https://crrev.com/7bec88d891d568be4392e3e5d74d04b79ff4e667
> Cr-Commit-Position: refs/heads/master@{#418182}

TBR=satorux@google.com,satorux@chromium.org,yamaguchi@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 641943 

Review-Url: https://codereview.chromium.org/2340433002
Cr-Commit-Position: refs/heads/master@{#418219}

[modify] https://crrev.com/dae37c463b67bc923d301e347177c7d5c874cce7/chromeos/disks/disk_mount_manager_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a07c4fb1ece6ee6c6a2170825208b1f873012a0a

commit a07c4fb1ece6ee6c6a2170825208b1f873012a0a
Author: yamaguchi <yamaguchi@chromium.org>
Date: Wed Sep 14 03:58:03 2016

Refactor disk_mount_manager_unittest by replacing gmock with a dedicated mock class of DiskMountManagerObserver.

This change is based on
https://codereview.chromium.org/2292473002/
which was once merged but reverted by
https://codereview.chromium.org/2340433002/

BUG= 641943 

Review-Url: https://codereview.chromium.org/2333983004
Cr-Commit-Position: refs/heads/master@{#418478}

[modify] https://crrev.com/a07c4fb1ece6ee6c6a2170825208b1f873012a0a/chromeos/disks/disk_mount_manager_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/48ec0aa3ee5b7ab7efe7738cd03b243bd5874c62

commit 48ec0aa3ee5b7ab7efe7738cd03b243bd5874c62
Author: yamaguchi <yamaguchi@chromium.org>
Date: Wed Sep 14 10:00:45 2016

Revert of Add a mock class for DiskMountManagerObserver. disk_mount_manager_observer_unittests. (patchset #4 id:60001 of https://codereview.chromium.org/2333983004/ )

Reason for revert:
Asan failure

Original issue's description:
> Refactor disk_mount_manager_unittest by replacing gmock with a dedicated mock class of DiskMountManagerObserver.
>
> This change is based on
> https://codereview.chromium.org/2292473002/
> which was once merged but reverted by
> https://codereview.chromium.org/2340433002/
>
> BUG= 641943 
>
> Committed: https://crrev.com/a07c4fb1ece6ee6c6a2170825208b1f873012a0a
> Cr-Commit-Position: refs/heads/master@{#418478}

TBR=satorux@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 641943 

Review-Url: https://codereview.chromium.org/2343593002
Cr-Commit-Position: refs/heads/master@{#418520}

[modify] https://crrev.com/48ec0aa3ee5b7ab7efe7738cd03b243bd5874c62/chromeos/disks/disk_mount_manager_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cafadf5bd103b96c300eed6aad4244837d9f6b76

commit cafadf5bd103b96c300eed6aad4244837d9f6b76
Author: yamaguchi <yamaguchi@chromium.org>
Date: Thu Sep 15 09:05:53 2016

Refactor disk_mount_manager_unittest by replacing gmock with a dedicated mock class of DiskMountManagerObserver.

This change is based on
https://codereview.chromium.org/2333983004/
which was once merged but reverted by
https://codereview.chromium.org/2343593002/

BUG= 641943 
TEST=chromeos_unittest build with is_asan and is_lsan = true in gn args, and run with ASAN_OPTIONS="detect_leaks=1 symbolize=1" environment variable.

Review-Url: https://codereview.chromium.org/2341923003
Cr-Commit-Position: refs/heads/master@{#418811}

[modify] https://crrev.com/cafadf5bd103b96c300eed6aad4244837d9f6b76/chromeos/disks/disk_mount_manager_unittest.cc

Status: Fixed (was: Started)

Comment 7 by dchan@chromium.org, Oct 7 2016

Labels: VerifyIn-55
Status: Verified (was: Fixed)
Closing bug as per comments #3 #4 and #5

Sign in to add a comment