Code review comment for lp:~nick-dedekind/mir/trusted_sessions

Just some quick notes:

1088 + * Copyright © 2013 Canonical Ltd.
...
3383 + * Copyright © 2013 Canonical Ltd.

I know this was first proposed months ago - but was it really written last year? And not changed this year?

~~~~

+TEST_F(MirClientTrustSessionAPITest, client_trust_session_api)
+{
+ struct ClientConfig : ClientConfigCommon
+ {
+ void exec()
+ {
+ mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this));
+
+ ASSERT_TRUE(connection != NULL);
+ EXPECT_TRUE(mir_connection_is_valid(connection));
+ EXPECT_STREQ("", mir_connection_get_error_message(connection));
+
+ mir_wait_for(mir_connection_start_trust_session(connection, __LINE__, trust_session_start_callback, NULL, this));
+ ASSERT_TRUE(trust_session != NULL);
+ EXPECT_EQ(started, 1);
+
+ mir_wait_for(mir_trust_session_stop(trust_session, trust_session_stop_callback, this));
+ EXPECT_EQ(stopped, 1);
+
+ mir_connection_release(connection);
+ }
+ } client_config;
+
+ launch_client_process(client_config);
+}

This test has an unclear name and asserts things that are unrelated to the purpose of the test. It would also be simpler using InProcessServer as its basis, not DefaultDisplayServerTestFixture (side note: that name has become misleading.) It is also overcomplicated in its use of management of connection. It should look like:

TEST_F(TrustSessionClientAPI, can_start_and_stop_a_trust_session)
{
    mir_wait_for(mir_connection_start_trust_session(connection, arbitrarty_base_session_pid, trust_session_start_callback, null_event_callback, this));
    ASSERT_THAT(trust_session, Ne(NULL));
    EXPECT_THAT(started, Eq(1));

    mir_wait_for(mir_trust_session_stop(trust_session, trust_session_stop_callback, this));
    EXPECT_THAT(stopped, Eq(1));
}

~~~~

+struct TrustSession : public testing::Test
+{
+ TrustSession()
+ {
+ }
+
+ testing::NiceMock<mtd::MockTrustSessionListener> trust_session_listener;
+ testing::NiceMock<mtd::MockSceneSession> trusted_helper;
+ testing::NiceMock<mtd::MockSceneSession> trusted_app1;
+ testing::NiceMock<mtd::MockSceneSession> trusted_app2;

NiceMock is inappropriate as these are the interactions under test.

+};
+}
+
+TEST_F(TrustSession, start_and_stop)

the name doesn't reflect what the test is about. There is no mention of adding and removing apps.

+{
+ using namespace testing;
+
+ auto shared_helper = mt::fake_shared(trusted_helper);
+ std::shared_ptr<ms::Session> shared_app1 = mt::fake_shared(trusted_app1);
+ std::shared_ptr<ms::Session> shared_app2 = mt::fake_shared(trusted_app2);
+
+ ms::TrustSessionImpl trust_session(shared_helper,
+ ms::a_trust_session(),
+ mt::fake_shared(trust_session_listener));

All the above initialization could be in the fixture

+ EXPECT_CALL(trusted_helper, begin_trust_session()).Times(1);
+ EXPECT_CALL(trusted_helper, end_trust_session()).Times(1);
+
+ EXPECT_CALL(trusted_app1, begin_trust_session()).Times(0);
+ EXPECT_CALL(trusted_app1, end_trust_session()).Times(0);
+
+ EXPECT_CALL(trusted_app2, begin_trust_session()).Times(0);
+ EXPECT_CALL(trusted_app2, end_trust_session()).Times(0);

These Times(0) are only relevant because of NiceMock

+ EXPECT_CALL(trust_session_listener, trusted_session_beginning(_, shared_app1)).Times(1);
+ EXPECT_CALL(trust_session_listener, trusted_session_beginning(_, shared_app2)).Times(1);
+
+ EXPECT_CALL(trust_session_listener, trusted_session_ending(_, shared_app1)).Times(1);
+ EXPECT_CALL(trust_session_listener, trusted_session_ending(_, shared_app2)).Times(1);
+
+ trust_session.start();
+ trust_session.add_trusted_child(shared_app1);
+ trust_session.add_trusted_child(shared_app2);
+ trust_session.stop();
+}

Really the test is:

TEST_F(TrustSession, trusted_child_apps_get_start_and_stop_notifications)
{
    EXPECT_CALL(trust_session_listener, trusted_session_beginning(_, shared_app1)).Times(1);
    EXPECT_CALL(trust_session_listener, trusted_session_beginning(_, shared_app2)).Times(1);

    EXPECT_CALL(trust_session_listener, trusted_session_ending(_, shared_app1)).Times(1);
    EXPECT_CALL(trust_session_listener, trusted_session_ending(_, shared_app2)).Times(1);

    trust_session.add_trusted_child(shared_app1);
    trust_session.add_trusted_child(shared_app2);
}

And the fixture:

struct TrustSession : public testing::Test
{
    testing::NiceMock<mtd::MockTrustSessionListener> trust_session_listener;
    mtd::MockSceneSession trusted_helper;
    mtd::MockSceneSession trusted_app1;
    mtd::MockSceneSession trusted_app2;

    std::shared_ptr<ms::Session> shared_helper = mt::fake_shared(trusted_helper);
    std::shared_ptr<ms::Session> shared_app1 = mt::fake_shared(trusted_app1);
    std::shared_ptr<ms::Session> shared_app2 = mt::fake_shared(trusted_app2);

    ms::TrustSessionImpl trust_session{shared_helper,
                                   ms::a_trust_session(),
                                   mt::fake_shared(trust_session_listener)};

    void SetUp()
    {
        InSequence seq;
        EXPECT_CALL(trusted_helper, begin_trust_session()).Times(1);
        EXPECT_CALL(trusted_helper, end_trust_session()).Times(1);

        trust_session.start();
    }

    void TearDown()
    {
        trust_session.stop();
    }
};