Merge lp:~vanvugt/mir/simplify-DisplayReport into lp:mir

[Daniel van Vugt]

Simplify DisplayReport, merging success/failure reports into a single
interface.

This is a marginal improvement to the coupling and scalability of DisplayReport. Some people may disagree with it, which I kind of understand. But if this isn't sufficiently better then I hope it gets you thinking about what else might be. Because pure virtual report classes which you have to implement every function of should not be growing so large or fast (I need to add to it soon).

[Alan Griffiths (alan-griffiths)]

I'm not saying that this report couldn't do with improvement but...

The reason for the "report" interfaces is that we can do more than simply logging events (although that is want we've done mostly - but IMO that is due to the early state of the code).

What I'd like to be able to provide is reporting code that can respond to a "ping" with a summary of current system activity - the state of outputs, connections, surfaces etc. I think this is something that we will need within half year.

The "report" interfaces should provide support for this tracking of the state of the system programmatically - which is far easier if the events that need identifying are distinct calls and not different text strings.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1295
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/563/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/490
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/486
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/96
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/289
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/289/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/292
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/292/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/96
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/96/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/128
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2519

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/563/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> I'm not saying that this report couldn't do with improvement but...
>
> The reason for the "report" interfaces is that we can do more than simply
> logging events (although that is want we've done mostly - but IMO that is due
> to the early state of the code).
>
> What I'd like to be able to provide is reporting code that can respond to a
> "ping" with a summary of current system activity - the state of outputs,
> connections, surfaces etc. I think this is something that we will need within
> half year.
>
> The "report" interfaces should provide support for this tracking of the state
> of the system programmatically - which is far easier if the events that need
> identifying are distinct calls and not different text strings.

Having looked closer I think the existing interface is too fined grained but that the MP swings too far in the opposite direction.

For example: There are a series of successful setup/construction calls that are really only useful for recording process via logging. But the fact of successful construction is probably worth distinguishing by a separate event.

Similarly, VT switch events are likely significant beyond providing a log of events.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Alan,
I understand and agree with your concerns. Although I'm not sure implementing N report functions M times is better than implementing just one report function M times with conditional logic.

The key requirement to improving things, I feel, is to arrive at a design where the report definition doesn't have to change for every new instrumentation point. Something less than O(N*M).

[Robert Carr (robertcarr)]

I think at the point where you have report->report_success(bool, char const* what), it might as well be
logger->log(is_error, message).

The current interface isn't great. I don't want to move in this direction though.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

I think I get the intent with the report pattern. And I suspect it works best for arbitrary report types when the input (interface) is minimal and simple.

Although I also think any report implementation class should be able to distinguish different events by string (or some other unique value).

[Alan Griffiths (alan-griffiths)]

> I think I get the intent with the report pattern. And I suspect it works best
> for arbitrary report types when the input (interface) is minimal and simple.
>
> Although I also think any report implementation class should be able to
> distinguish different events by string (or some other unique value).

I think any report implementation should implement handlers for each of the different events being reported on. There's a mechanism (pure virtual functions) in the language that enforces this at compile time. I prefer that to detecting a failure to match strings correctly in a test harness.

[Andreas Pokorny (andreas-pokorny)]

I am used to systems that have some form of log macro that can be enabled/disabled and runtime and compile time. To say the least it is simple to add further log messages. The solution in mir is quite different. I am curious what led to that solution?

[Daniel van Vugt (vanvugt)]

Andreas,
Me too. And I was arguing for that kind of simple logging before we had "reports". It has many benefits that you touched on. However if you design a "report" interface carefully then it's potentially very useful in instrumenting to data sinks that aren't necessarily logging (LTTng et al). Still, I feel the right answer is to have both. Because "reports" as they are today in Mir are too brittle (their design is uncomfortably coupled to the logic of another module) and slow to evolve (multiple classes plus tests) compared to adding new log messages.

Back on the subject of this proposal, I'm happy to reject it if we get a third disapproving review :)

[Andreas Pokorny (andreas-pokorny)]

Yes I see the advantages of the report interfaces too, and in that MP you would kind of limit those. The other question would be - are those methods in the report interface useful for understanding whats happening then they should definitely stay in that way. If not then there is a need for a separate logging facility.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> Andreas,
> Me too. And I was arguing for that kind of simple logging before we had
> "reports". It has many benefits that you touched on. However if you design a
> "report" interface carefully then it's potentially very useful in
> instrumenting to data sinks that aren't necessarily logging (LTTng et al).
> Still, I feel the right answer is to have both.

I've no issue with "having both" - indeed that's what I understood to be the intent when we discussed it at the sprint two years ago. For example, report->informational_log("...") or report->debug_log("...").

It is possible to debate where we draw the line between the events that should be reported and information that should be logged, but this report currently has too much of the former and this MP has too much of the latter.

Unmerged revisions

1295. By Daniel van Vugt on 2013-12-19

Port Android DisplayReports

1294. By Daniel van Vugt on 2013-12-19

Merge latest development-branch.

1293. By Daniel van Vugt on 2013-12-19

Initial attempt at simplifying DisplayReport

=== modified file 'include/platform/mir/graphics/display_report.h'
--- include/platform/mir/graphics/display_report.h	2013-08-28 03:41:48 +0000
+++ include/platform/mir/graphics/display_report.h	2013-12-19 09:18:52 +0000
@@ -29,17 +29,10 @@
 class DisplayReport
 {
 public:
-
-    virtual void report_successful_setup_of_native_resources() = 0;
-    virtual void report_successful_egl_make_current_on_construction() = 0;
-    virtual void report_successful_egl_buffer_swap_on_construction() = 0;
-    virtual void report_successful_display_construction() = 0;
+    virtual void report_success(bool success, char const* what) = 0;
     virtual void report_egl_configuration(EGLDisplay disp, EGLConfig cfg) = 0;
     /* gbm specific */
-    virtual void report_successful_drm_mode_set_crtc_on_construction() = 0;
     virtual void report_drm_master_failure(int error) = 0;
-    virtual void report_vt_switch_away_failure() = 0;
-    virtual void report_vt_switch_back_failure() = 0;
     /* android specific */
     virtual void report_hwc_composition_in_use(int major, int minor) = 0;
     virtual void report_gpu_composition_in_use() = 0;
 
=== modified file 'include/platform/mir/graphics/null_display_report.h'
--- include/platform/mir/graphics/null_display_report.h	2013-08-28 03:41:48 +0000
+++ include/platform/mir/graphics/null_display_report.h	2013-12-19 09:18:52 +0000
@@ -31,15 +31,8 @@
 class NullDisplayReport : public graphics::DisplayReport
 {
   public:
-
-    virtual void report_successful_setup_of_native_resources();
-    virtual void report_successful_egl_make_current_on_construction();
-    virtual void report_successful_egl_buffer_swap_on_construction();
-    virtual void report_successful_drm_mode_set_crtc_on_construction();
-    virtual void report_successful_display_construction();
+    virtual void report_success(bool success, char const* what);
     virtual void report_drm_master_failure(int error);
-    virtual void report_vt_switch_away_failure();
-    virtual void report_vt_switch_back_failure();
     virtual void report_hwc_composition_in_use(int major, int minor);
     virtual void report_gpu_composition_in_use();
     virtual void report_egl_configuration(EGLDisplay disp, EGLConfig cfg);
 
=== modified file 'include/test/mir_test_doubles/mock_display_report.h'
--- include/test/mir_test_doubles/mock_display_report.h	2013-05-10 18:16:51 +0000
+++ include/test/mir_test_doubles/mock_display_report.h	2013-12-19 09:18:52 +0000
@@ -33,14 +33,8 @@
 class MockDisplayReport : public graphics::DisplayReport
 {
 public:
-    MOCK_METHOD0(report_successful_setup_of_native_resources, void());
-    MOCK_METHOD0(report_successful_egl_make_current_on_construction, void());
-    MOCK_METHOD0(report_successful_egl_buffer_swap_on_construction, void());
-    MOCK_METHOD0(report_successful_drm_mode_set_crtc_on_construction, void());
-    MOCK_METHOD0(report_successful_display_construction, void());
+    MOCK_METHOD2(report_success, void(bool, char const*));
     MOCK_METHOD1(report_drm_master_failure, void(int));
-    MOCK_METHOD0(report_vt_switch_away_failure, void());
-    MOCK_METHOD0(report_vt_switch_back_failure, void());
     MOCK_METHOD2(report_hwc_composition_in_use, void(int,int));
     MOCK_METHOD0(report_gpu_composition_in_use, void());
     MOCK_METHOD2(report_egl_configuration, void(EGLDisplay,EGLConfig));
 
=== modified file 'src/platform/graphics/android/android_display.cpp'
--- src/platform/graphics/android/android_display.cpp	2013-12-17 18:24:51 +0000
+++ src/platform/graphics/android/android_display.cpp	2013-12-19 09:18:52 +0000
@@ -39,12 +39,12 @@
       display_buffer{display_builder->create_display_buffer(display_device, gl_context)},
       current_configuration{display_buffer->view_area().size}
 {
-    display_report->report_successful_setup_of_native_resources();
+    display_report->report_success(true, "setup of native resources");
 
     gl_context.make_current();
 
-    display_report->report_successful_egl_make_current_on_construction();
-    display_report->report_successful_display_construction();
+    display_report->report_success(true, "set context current on construction");
+    display_report->report_success(true, "construction");
 }
 
 void mga::AndroidDisplay::for_each_display_buffer(std::function<void(mg::DisplayBuffer&)> const& f)
 
=== modified file 'src/platform/graphics/mesa/display_buffer.cpp'
--- src/platform/graphics/mesa/display_buffer.cpp	2013-12-17 18:24:51 +0000
+++ src/platform/graphics/mesa/display_buffer.cpp	2013-12-19 09:18:52 +0000
@@ -112,11 +112,11 @@
 {
     egl.setup(platform->gbm, surface_gbm.get(), shared_context);
 
-    listener->report_successful_setup_of_native_resources();
+    listener->report_success(true, "setup of native resources");
 
     make_current();
 
-    listener->report_successful_egl_make_current_on_construction();
+    listener->report_success(true, "set context current on construction");
 
     ensure_egl_image_extensions();
 
@@ -125,7 +125,7 @@
     if (!egl.swap_buffers())
         BOOST_THROW_EXCEPTION(std::runtime_error("Failed to perform initial surface buffer swap"));
 
-    listener->report_successful_egl_buffer_swap_on_construction();
+    listener->report_success(true, "egl buffer swap on construction");
 
     last_flipped_bufobj = get_front_buffer_object();
     if (!last_flipped_bufobj)
@@ -139,8 +139,8 @@
 
     egl.release_current();
 
-    listener->report_successful_drm_mode_set_crtc_on_construction();
-    listener->report_successful_display_construction();
+    listener->report_success(true, "drm mode setup on construction");
+    listener->report_success(true, "construction");
     egl.report_egl_configuration(
         [&listener] (EGLDisplay disp, EGLConfig cfg)
         {
 
=== modified file 'src/platform/graphics/mesa/linux_virtual_terminal.cpp'
--- src/platform/graphics/mesa/linux_virtual_terminal.cpp	2013-12-17 18:24:51 +0000
+++ src/platform/graphics/mesa/linux_virtual_terminal.cpp	2013-12-19 09:18:52 +0000
@@ -138,7 +138,7 @@
             if (!active)
             {
                 if (!switch_back())
-                    report->report_vt_switch_back_failure();
+                    report->report_success(false, "switch back to Mir VT");
                 fops->ioctl(vt_fd.fd(), VT_RELDISP, VT_ACKACQ);
                 active = true;
             }
@@ -156,7 +156,7 @@
                 else
                 {
                     action = disallow_switch;
-                    report->report_vt_switch_away_failure();
+                    report->report_success(false, "switch away from Mir VT");
                 }
 
                 fops->ioctl(vt_fd.fd(), VT_RELDISP, action);
 
=== modified file 'src/platform/graphics/null_display_report.cpp'
--- src/platform/graphics/null_display_report.cpp	2013-08-28 03:41:48 +0000
+++ src/platform/graphics/null_display_report.cpp	2013-12-19 09:18:52 +0000
@@ -20,14 +20,8 @@
 
 namespace mg = mir::graphics;
 
-void mg::NullDisplayReport::report_successful_setup_of_native_resources() {}
-void mg::NullDisplayReport::report_successful_egl_make_current_on_construction() {}
-void mg::NullDisplayReport::report_successful_egl_buffer_swap_on_construction() {}
-void mg::NullDisplayReport::report_successful_drm_mode_set_crtc_on_construction() {}
-void mg::NullDisplayReport::report_successful_display_construction() {}
+void mg::NullDisplayReport::report_success(bool, char const*) {}
 void mg::NullDisplayReport::report_drm_master_failure(int) {}
-void mg::NullDisplayReport::report_vt_switch_away_failure() {}
-void mg::NullDisplayReport::report_vt_switch_back_failure() {}
 void mg::NullDisplayReport::report_hwc_composition_in_use(int, int) {}
 void mg::NullDisplayReport::report_gpu_composition_in_use() {}
 void mg::NullDisplayReport::report_egl_configuration(EGLDisplay, EGLConfig) {}
 
=== modified file 'src/server/logging/display_report.cpp'
--- src/server/logging/display_report.cpp	2013-12-17 18:24:51 +0000
+++ src/server/logging/display_report.cpp	2013-12-19 09:18:52 +0000
@@ -24,6 +24,11 @@
 
 namespace ml=mir::logging;
 
+namespace
+{
+    const char * const component = "graphics";
+}
+
 ml::DisplayReport::DisplayReport(const std::shared_ptr<Logger>& logger) : logger(logger)
 {
 }
@@ -32,36 +37,13 @@
 {
 }
 
-const char* ml::DisplayReport::component()
-{
-    static const char* s = "graphics";
-    return s;
-}
-
-
-void ml::DisplayReport::report_successful_setup_of_native_resources()
-{
-    logger->log(Logger::informational, "Successfully setup native resources.", component());
-}
-
-void ml::DisplayReport::report_successful_egl_make_current_on_construction()
-{
-    logger->log(Logger::informational, "Successfully made egl context current on construction.", component());
-}
-
-void ml::DisplayReport::report_successful_egl_buffer_swap_on_construction()
-{
-    logger->log(Logger::informational, "Successfully performed egl buffer swap on construction.", component());
-}
-
-void ml::DisplayReport::report_successful_drm_mode_set_crtc_on_construction()
-{
-    logger->log(Logger::informational, "Successfully performed drm mode setup on construction.", component());
-}
-
-void ml::DisplayReport::report_successful_display_construction()
-{
-    logger->log(Logger::informational, "Successfully finished construction.", component());
+void ml::DisplayReport::report_success(bool success, char const* what)
+{
+    std::string msg = success ? "Successful " : "Failed ";
+    msg += what;
+    Logger::Severity sev = success ? Logger::informational :
+                                     Logger::error;
+    logger->log(sev, msg, component);
 }
 
 void ml::DisplayReport::report_drm_master_failure(int error)
@@ -71,29 +53,19 @@
     if (error == EPERM || error == EACCES)
         ss << " Try running Mir with root privileges.";
 
-    logger->log(Logger::warning, ss.str(), component());
-}
-
-void ml::DisplayReport::report_vt_switch_away_failure()
-{
-    logger->log(Logger::warning, "Failed to switch away from Mir VT.", component());
-}
-
-void ml::DisplayReport::report_vt_switch_back_failure()
-{
-    logger->log(Logger::warning, "Failed to switch back to Mir VT.", component());
+    logger->log(Logger::warning, ss.str(), component);
 }
 
 void ml::DisplayReport::report_hwc_composition_in_use(int major, int minor)
 {
     std::stringstream ss;
     ss << "HWC version " << major << "." << minor << " in use for display.";
-    logger->log(Logger::informational, ss.str(), component());
+    logger->log(Logger::informational, ss.str(), component);
 }
 
 void ml::DisplayReport::report_gpu_composition_in_use()
 {
-    logger->log(Logger::informational, "GPU backup in use for display.", component());
+    logger->log(Logger::informational, "GPU backup in use for display.", component);
 }
 
 void ml::DisplayReport::report_egl_configuration(EGLDisplay disp, EGLConfig config)
@@ -143,12 +115,12 @@
     };
     #undef STRMACRO
 
-    logger->log(Logger::informational, "Display EGL Configuration:", component());
+    logger->log(Logger::informational, "Display EGL Configuration:", component);
     for( auto &i : egl_string_mapping)
     {
         EGLint value;
         eglGetConfigAttrib(disp, config, i.val, &value);
         logger->log(Logger::informational, 
-            "    [" + i.name + "] : " + std::to_string(value), component());
+            "    [" + i.name + "] : " + std::to_string(value), component);
     }
 }
 
=== modified file 'src/server/logging/display_report.h'
--- src/server/logging/display_report.h	2013-12-17 18:24:51 +0000
+++ src/server/logging/display_report.h	2013-12-19 09:18:52 +0000
@@ -33,20 +33,11 @@
 class DisplayReport : public graphics::DisplayReport
 {
   public:
-
-    static const char* component();
-
     DisplayReport(const std::shared_ptr<Logger>& logger);
     virtual ~DisplayReport();
 
-    virtual void report_successful_setup_of_native_resources();
-    virtual void report_successful_egl_make_current_on_construction();
-    virtual void report_successful_egl_buffer_swap_on_construction();
-    virtual void report_successful_drm_mode_set_crtc_on_construction();
-    virtual void report_successful_display_construction();
+    virtual void report_success(bool success, char const* what);
     virtual void report_drm_master_failure(int error);
-    virtual void report_vt_switch_away_failure();
-    virtual void report_vt_switch_back_failure();
     virtual void report_hwc_composition_in_use(int major, int minor);
     virtual void report_gpu_composition_in_use();
     virtual void report_egl_configuration(EGLDisplay disp, EGLConfig cfg);
 
=== modified file 'tests/unit-tests/graphics/android/test_android_fb.cpp'
--- tests/unit-tests/graphics/android/test_android_fb.cpp	2013-12-17 18:24:51 +0000
+++ tests/unit-tests/graphics/android/test_android_fb.cpp	2013-12-19 09:18:52 +0000
@@ -154,15 +154,19 @@
 TEST_F(AndroidDisplayTest, display_logging)
 {
     using namespace testing;
-    EXPECT_CALL(*mock_display_report, report_successful_setup_of_native_resources())
+
+    EXPECT_CALL(*mock_display_report,
+                report_success(true, StrEq("setup of native resources")))
         .Times(1);
     EXPECT_CALL(*mock_display_report, report_egl_configuration(_,_))
         .Times(1);
-    EXPECT_CALL(*mock_display_report, report_successful_egl_make_current_on_construction())
-        .Times(1);
-    EXPECT_CALL(*mock_display_report, report_successful_display_construction())
-        .Times(1);
-
+    EXPECT_CALL(*mock_display_report,
+                report_success(true, StrEq("set context current on construction")))
+        .Times(1);
+    EXPECT_CALL(*mock_display_report,
+                report_success(true, StrEq("construction")))
+        .Times(1);
+ 
     mga::AndroidDisplay display(stub_db_factory, mock_display_report);
 }
 
@@ -170,14 +174,17 @@
 {
     using namespace testing;
 
-    EXPECT_CALL(*mock_display_report, report_successful_setup_of_native_resources())
+    EXPECT_CALL(*mock_display_report,
+                report_success(true, StrEq("setup of native resources")))
         .Times(1);
     EXPECT_CALL(mock_egl, eglMakeCurrent(dummy_display, _, _, _))
         .Times(1)
         .WillOnce(Return(EGL_FALSE));
-    EXPECT_CALL(*mock_display_report, report_successful_egl_make_current_on_construction())
+    EXPECT_CALL(*mock_display_report,
+                report_success(_, StrEq("set context current on construction")))
         .Times(0);
-    EXPECT_CALL(*mock_display_report, report_successful_display_construction())
+    EXPECT_CALL(*mock_display_report,
+                report_success(_, StrEq("construction")))
         .Times(0);
 
     EXPECT_THROW({
@@ -189,11 +196,7 @@
 {
     using namespace testing;
 
-    EXPECT_CALL(*mock_display_report, report_successful_setup_of_native_resources())
-        .Times(0);
-    EXPECT_CALL(*mock_display_report, report_successful_egl_make_current_on_construction())
-        .Times(0);
-    EXPECT_CALL(*mock_display_report, report_successful_display_construction())
+    EXPECT_CALL(*mock_display_report, report_success(_,_))
         .Times(0);
 
     EXPECT_CALL(mock_egl, eglCreatePbufferSurface(_,_,_))
 
=== modified file 'tests/unit-tests/graphics/mesa/test_display.cpp'
--- tests/unit-tests/graphics/mesa/test_display.cpp	2013-12-17 18:24:51 +0000
+++ tests/unit-tests/graphics/mesa/test_display.cpp	2013-12-19 09:18:52 +0000
@@ -492,24 +492,27 @@
 {
     using namespace ::testing;
 
-    EXPECT_CALL(
-        *mock_report,
-        report_successful_setup_of_native_resources()).Times(Exactly(1));
-    EXPECT_CALL(
-        *mock_report,
-        report_successful_egl_make_current_on_construction()).Times(Exactly(1));
-
-    EXPECT_CALL(
-        *mock_report,
-        report_successful_egl_buffer_swap_on_construction()).Times(Exactly(1));
-
-    EXPECT_CALL(
-        *mock_report,
-        report_successful_drm_mode_set_crtc_on_construction()).Times(Exactly(1));
-
-    EXPECT_CALL(
-        *mock_report,
-        report_successful_display_construction()).Times(Exactly(1));
+    // Is there really any value in these expectations?...
+
+    EXPECT_CALL(*mock_report,
+                report_success(true, StrEq("setup of native resources")))
+        .Times(Exactly(1));
+
+    EXPECT_CALL(*mock_report,
+                report_success(true, StrEq("set context current on construction")))
+        .Times(Exactly(1));
+
+    EXPECT_CALL(*mock_report,
+                report_success(true, StrEq("egl buffer swap on construction")))
+        .Times(Exactly(1));
+
+    EXPECT_CALL(*mock_report,
+                report_success(true, StrEq("drm mode setup on construction")))
+        .Times(Exactly(1));
+
+    EXPECT_CALL(*mock_report,
+                report_success(true, StrEq("construction")))
+        .Times(Exactly(1));
 
     EXPECT_CALL(
         *mock_report,
@@ -521,68 +524,36 @@
                         mock_report);
 }
 
-TEST_F(MesaDisplayTest, outputs_correct_string_for_successful_setup_of_native_resources)
-{
-    using namespace ::testing;
-
-    auto logger = std::make_shared<MockLogger>();
-    auto reporter = std::make_shared<ml::DisplayReport>(logger);
-
-    EXPECT_CALL(
-        *logger,
-        log(Eq(ml::Logger::informational),
-            StrEq("Successfully setup native resources."),
-            StrEq("graphics"))).Times(Exactly(1));
-
-    reporter->report_successful_setup_of_native_resources();
-}
-
-TEST_F(MesaDisplayTest, outputs_correct_string_for_successful_egl_make_current_on_construction)
-{
-    using namespace ::testing;
-
-    auto logger = std::make_shared<MockLogger>();
-    auto reporter = std::make_shared<ml::DisplayReport>(logger);
-
-    EXPECT_CALL(
-        *logger,
-        log(Eq(ml::Logger::informational),
-            StrEq("Successfully made egl context current on construction."),
-            StrEq("graphics"))).Times(Exactly(1));
-
-    reporter->report_successful_egl_make_current_on_construction();
-}
-
-TEST_F(MesaDisplayTest, outputs_correct_string_for_successful_egl_buffer_swap_on_construction)
-{
-    using namespace ::testing;
-
-    auto logger = std::make_shared<MockLogger>();
-    auto reporter = std::make_shared<ml::DisplayReport>(logger);
-
-    EXPECT_CALL(
-        *logger,
-        log(Eq(ml::Logger::informational),
-            StrEq("Successfully performed egl buffer swap on construction."),
-            StrEq("graphics"))).Times(Exactly(1));
-
-    reporter->report_successful_egl_buffer_swap_on_construction();
-}
-
-TEST_F(MesaDisplayTest, outputs_correct_string_for_successful_drm_mode_set_crtc_on_construction)
-{
-    using namespace ::testing;
-
-    auto logger = std::make_shared<MockLogger>();
-    auto reporter = std::make_shared<ml::DisplayReport>(logger);
-
-    EXPECT_CALL(
-        *logger,
-        log(Eq(ml::Logger::informational),
-            StrEq("Successfully performed drm mode setup on construction."),
-            StrEq("graphics"))).Times(Exactly(1));
-
-    reporter->report_successful_drm_mode_set_crtc_on_construction();
+TEST_F(MesaDisplayTest, outputs_correct_success_string)
+{
+    using namespace ::testing;
+
+    auto logger = std::make_shared<MockLogger>();
+    auto reporter = std::make_shared<ml::DisplayReport>(logger);
+
+    EXPECT_CALL(
+        *logger,
+        log(Eq(ml::Logger::informational),
+            StrEq("Successful something"),
+            StrEq("graphics"))).Times(Exactly(1));
+
+    reporter->report_success(true, "something");
+}
+
+TEST_F(MesaDisplayTest, outputs_correct_failure_string)
+{
+    using namespace ::testing;
+
+    auto logger = std::make_shared<MockLogger>();
+    auto reporter = std::make_shared<ml::DisplayReport>(logger);
+
+    EXPECT_CALL(
+        *logger,
+        log(Eq(ml::Logger::error),
+            StrEq("Failed something"),
+            StrEq("graphics"))).Times(Exactly(1));
+
+    reporter->report_success(false, "something");
 }
 
 TEST_F(MesaDisplayTest, constructor_throws_if_egl_mesa_drm_image_not_supported)
 
=== modified file 'tests/unit-tests/graphics/mesa/test_linux_virtual_terminal.cpp'
--- tests/unit-tests/graphics/mesa/test_linux_virtual_terminal.cpp	2013-12-17 18:24:51 +0000
+++ tests/unit-tests/graphics/mesa/test_linux_virtual_terminal.cpp	2013-12-19 09:18:52 +0000
@@ -430,12 +430,14 @@
     set_up_expectations_for_switch_handler(SIGUSR1);
 
     /* First switch away attempt */
-    EXPECT_CALL(mock_report, report_vt_switch_away_failure());
+    EXPECT_CALL(mock_report,
+                report_success(false, StrEq("switch away from Mir VT")));
     EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, VT_RELDISP,
                                  MatcherCast<int>(disallow_switch)));
 
     /* Second switch away attempt */
-    EXPECT_CALL(mock_report, report_vt_switch_away_failure());
+    EXPECT_CALL(mock_report,
+                report_success(false, StrEq("switch away from Mir VT")));
     EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, VT_RELDISP,
                                  MatcherCast<int>(disallow_switch)));
 
@@ -472,7 +474,8 @@
     EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, VT_RELDISP, allow_switch));
 
     /* Switch back */
-    EXPECT_CALL(mock_report, report_vt_switch_back_failure());
+    EXPECT_CALL(mock_report,
+                report_success(false, StrEq("switch back to Mir VT")));
     EXPECT_CALL(mock_fops, ioctl(fake_vt_fd, VT_RELDISP, VT_ACKACQ));
 
     set_up_expectations_for_vt_teardown();