Mir

Code review comment for lp:~kdub/mir/no-rendering-operator

Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

271 + EXPECT_EQ(buf1.use_count(), use_count1);
272 + EXPECT_EQ(buf2.use_count(), use_count2);

Expectations are reversed (we can use EXPECT_THAT() which is less error prone).

But, taking a step back, I see two issues with the test:

1. It's checking the number of users *before* posting, whereas we care about the number of users *after* posting, but that's OK for a unit test (that is we are modelling a post_update() that doesn't mess with the buffers, which is reasonable).

2. The test is too strict and dependent on the internals. We are checking that the number of users at posting time is the same as the number of users at rendering time, but that's not what we really care about. What we want to check is that the buffer is held alive until after post_update(), i.e., there is at least one user (besides the test function) at that point:

EXPECT_THAT(buf2.use_count() - 1, Ge(1));
EXPECT_THAT(buf2.use_count() - 1, Ge(1));

review: Needs Fixing

« Back to merge proposal