Mir

Merge lp:~vanvugt/mir/fix-1390312 into lp:mir

  1. fix-1390312
  2. Merge into development-branch
Proposed by Daniel van Vugt
Status: Merged
Approved by: Kevin DuBois
Approved revision: no longer in the source branch.
Merged at revision: 2046
Proposed branch: lp:~vanvugt/mir/fix-1390312
Merge into: lp:mir
Diff against target: 83 lines (+9/-19)
1 file modified
tests/unit-tests/scene/test_application_session.cpp (+9/-19)
To merge this branch: bzr merge lp:~vanvugt/mir/fix-1390312
Reviewer Review Type Date Requested Status
Kevin DuBois (community) Approve
Alan Griffiths Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+241040@code.launchpad.net

Commit message

Silence gmock warnings in ApplicationSession unit-tests (LP: #1390312)

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:2039
http://jenkins.qa.ubuntu.com/job/mir-ci/2032/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/28
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/28
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/7/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/21
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/21/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/7
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/7/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3302/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15539

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2032/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

failure is lp:1390388

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:2039
http://jenkins.qa.ubuntu.com/job/mir-ci/2042/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/39
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/39
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/19
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/31
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/31/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/19
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/19/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3314
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15593

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2042/rebuild

review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

I know this is an attempt to clean existing mess (and it may be the simplest fix) but...

I wish folks would simply not mock the functions they are not interested in instead of mocking them and then setting weak expectations.

Expectations set in a test should be checking what the test is about (and not just silencing warnings that would never have been generated if the function hadn't been mocked).

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Firstly, these tests are a mess already: they do not clearly identify what is being tested or distinguish between setup and expectation. For example,

   TEST(ApplicationSession, create_and_destroy_surface)

This is poorly named as "create_and_destroy_surface" doesn't tell us what is being tested.

A few lines later we have:

    mtd::MockSurfaceCoordinator surface_coordinator;

    EXPECT_CALL(surface_coordinator, add_surface(_, _))
        .WillOnce(Return(mock_surface));

I *think* this is a test expectation - although having expectations on two object (surface_coordinator and listener suggests we're testing two criteria, not one.

We could have:

   TEST(ApplicationSession, uses_surface_coordinator_to_create_and_destroy_surface)
   TEST(ApplicationSession, notifies_listener_surface_created_and_destroyed)

with appropriate expectations.

/rant off

~~~~

with the above proviso that the tests are an existing mess I think adding expectations to the tests is justified, but they should be stronger. E.g.

30 + EXPECT_CALL(surface_coordinator, remove_surface(_))
31 + .Times(AnyNumber());

    EXPECT_CALL(surface_coordinator, remove_surface(WeakPtrTo(mock_surface)))
        .Times(1);

~~~~

As we are rarely interested in *any* calls to surfaces...

8 - return std::make_shared<mtd::MockSurface>();
9 + using namespace ::testing;
10 + auto surface = std::make_shared<mtd::MockSurface>();
11 + EXPECT_CALL(*surface, add_observer(_))
12 + .Times(AnyNumber());
13 + return surface;

is better as:

    return std::make_shared<testing::NiceMock<mtd::MockSurface>>();

review: Needs Fixing
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Don't shoot the messenger. Happy to carry on a little bit with the clean up, but I didn't cause it :)

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:2042
http://jenkins.qa.ubuntu.com/job/mir-ci/2062/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/65
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/65
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/45
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/51
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/51/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/45
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/45/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3338
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15665

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2062/rebuild

review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

17 - mtd::MockSurfaceCoordinator surface_coordinator;
18 + NiceMock<mtd::MockSurfaceCoordinator> surface_coordinator;
19
20 EXPECT_CALL(surface_coordinator, add_surface(_, _))
21 .WillOnce(Return(mock_surface));

I've already noted the confused nature of this test. But this doesn't help. It should follow one of two options:

1. interaction with surface_coordinator is part of the "setup" and we should have (as we do elsewhere):

   NiceMock<mtd::MockSurfaceCoordinator> surface_coordinator;
   ON_CALL(surface_coordinator, add_surface(_,_)).WillByDefault(Return(mock_surface));

2. interaction with surface_coordinator is part of the "expectation" and we should have:

    mtd::MockSurfaceCoordinator surface_coordinator;
    EXPECT_CALL(surface_coordinator, remove_surface(WeakPtrTo(mock_surface)))
        .Times(1);

But the confusion is pre-existing and the rest fine.

review: Approve
Revision history for this message
Kevin DuBois (kdub) wrote :

I agree that this test needs some picking apart to make it clearer, but the cleanup seems like a step in the right direction. I've read a gmock comment that gmock intends to change the default mock behavior to NiceMock... but not as of 1.7 yet.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tests/unit-tests/scene/test_application_session.cpp'
2--- tests/unit-tests/scene/test_application_session.cpp 2014-10-01 06:25:56 +0000
3+++ tests/unit-tests/scene/test_application_session.cpp 2014-11-11 04:02:58 +0000
4@@ -43,7 +43,7 @@
5 {
6 static std::shared_ptr<mtd::MockSurface> make_mock_surface()
7 {
8- return std::make_shared<mtd::MockSurface>();
9+ return std::make_shared<testing::NiceMock<mtd::MockSurface> >();
10 }
11
12 class MockSnapshotStrategy : public ms::SnapshotStrategy
13@@ -84,7 +84,7 @@
14 auto mock_surface = make_mock_surface();
15
16 mtd::NullEventSink sender;
17- mtd::MockSurfaceCoordinator surface_coordinator;
18+ NiceMock<mtd::MockSurfaceCoordinator> surface_coordinator;
19
20 EXPECT_CALL(surface_coordinator, add_surface(_, _))
21 .WillOnce(Return(mock_surface));
22@@ -116,11 +116,9 @@
23 auto mock_surface = make_mock_surface();
24
25 mtd::NullEventSink sender;
26- mtd::MockSurfaceCoordinator surface_coordinator;
27+ NiceMock<mtd::MockSurfaceCoordinator> surface_coordinator;
28 ON_CALL(surface_coordinator, add_surface(_,_)).WillByDefault(Return(mock_surface));
29
30- EXPECT_CALL(surface_coordinator, add_surface(_, _));
31-
32 mtd::MockSessionListener listener;
33 EXPECT_CALL(listener, surface_created(_, _)).Times(1);
34 EXPECT_CALL(listener, destroying_surface(_, _)).Times(1);
35@@ -144,17 +142,11 @@
36 using namespace ::testing;
37
38 mtd::NullEventSink sender;
39- mtd::MockSurfaceCoordinator surface_coordinator;
40+ NiceMock<mtd::MockSurfaceCoordinator> surface_coordinator;
41
42- {
43- InSequence seq;
44- EXPECT_CALL(surface_coordinator, add_surface(_, _)).Times(1)
45- .WillOnce(Return(make_mock_surface()));
46- EXPECT_CALL(surface_coordinator, add_surface(_, _)).Times(1)
47- .WillOnce(Return(make_mock_surface()));
48- EXPECT_CALL(surface_coordinator, add_surface(_, _)).Times(1)
49- .WillOnce(Return(make_mock_surface()));
50- }
51+ EXPECT_CALL(surface_coordinator, add_surface(_, _))
52+ .Times(3)
53+ .WillRepeatedly(Return(make_mock_surface()));
54
55 ms::ApplicationSession app_session(
56 mt::fake_shared(surface_coordinator),
57@@ -190,7 +182,7 @@
58 mtd::NullEventSink sender;
59 auto mock_surface = make_mock_surface();
60
61- mtd::MockSurfaceCoordinator surface_coordinator;
62+ NiceMock<mtd::MockSurfaceCoordinator> surface_coordinator;
63 ON_CALL(surface_coordinator, add_surface(_, _)).WillByDefault(Return(mock_surface));
64
65 ms::ApplicationSession app_session(
66@@ -201,8 +193,6 @@
67 std::make_shared<ms::NullSessionListener>(),
68 mt::fake_shared(sender));
69
70- EXPECT_CALL(surface_coordinator, add_surface(_, _));
71-
72 {
73 InSequence seq;
74 EXPECT_CALL(*mock_surface, hide()).Times(1);
75@@ -264,7 +254,7 @@
76 {
77 using namespace ::testing;
78
79- mtd::MockSurfaceCoordinator surface_coordinator;
80+ NiceMock<mtd::MockSurfaceCoordinator> surface_coordinator;
81 mtd::NullEventSink sender;
82 auto const default_surface = make_mock_surface();
83 auto const default_surface_buffer_access =

Subscribers

People subscribed via source and target branches