[Webkit-unassigned] [Bug 68572] Create unit tests for LayerChromium

Thu Sep 22 18:47:46 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=68572

--- Comment #7 from Shawn Singh <shawnsingh at chromium.org>  2011-09-22 18:47:46 PST ---
(From update of attachment 108272)
Thanks for the comments.  I wasnt intending to land disabled tests or messy FIXME comments.  That was my (apparently unsuccessful) way of asking questions in-line with the code. Would it be better if I just put a bullet list of issues in a separate post on the bug?   How do you want me to communicate those points next time?

Anyway, I'll fix the style issues, and rest of my responses are inline.  Most of my responses will need one more pass of feedback from people to decide how to proceed.  Thanks!

View in context: https://bugs.webkit.org/attachment.cgi?id=108272&action=review

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:34
>> +using ::testing::_;
> 
> what is this?

This is gtest's symbol to represent "any parameter" in an EXPECT_CALL macro - when we expect a mocked function to be called, but we don't care what parameters are passed to it.  This is used with the silentDelegate.  If we end up removing silentDelegate, we can remove this, too.   Or if you prefer I can just have one line that says "using namespace ::testing" instead.

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:83
>> +        EXPECT_CALL(silentDelegate, notifySyncRequired()).Times(AnyNumber());
> 
> isn't this exactly the same as passing 0 in for the delegate?

in my opinion -  A "mock delegate" and "no delegate at all" are not the same - LayerChromium should believe it has a working delegate, but we don't care how it uses the delegate.   In our particular implementation of LayerChromium, yes they would produce the same "test results", but that's only a side effect of how we programmed the LayerChromium.  For example, if later we find its useful to add an early-exit if m_delegate is NULL...  Then we may not be testing what we think we are testing.  Using a stub seems less brittle to me.  Thoughts?

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:86
>> +    void explicitlyDestroyLayer(PassRefPtr<LayerChromium> layer) const
> 
> This appears to be redundant with LayerChromiumWithInstrumentedDestructor.  Is there any reason to have both?

I think its important that we directly test LayerChromium when possible.  Testing the subclass could be misleading if we introduce errors into the subclass.  However, to make the leak test, we had no choice but to passively check the destructor is called, and that should be the only test that uses this subclass.

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:156
>> +    RefPtr<LayerChromium> testLayer, parent, child, child1, child2, child3, child4, grandChild1, grandChild2, grandChild3;
> 
> While I appreciate the sentiment, I actually think doing this makes the tests much harder to read.  It still just just as many lines of code to instantiate each LayerChromium for a given test case (one) and you have to jump through unnatural hoops in order to drop references, etc.  I think you should instantiate the objects you need for a test within that test.

OK, will do =)

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:160
>> +class LayerChromiumTest_WORK_IN_PROGRESS : public LayerChromiumTest {
> 
> I don't understand what the point of this is - do you intend to check this in? does the _WORK_IN_PROGRESS suffix have some special meaning to the tools, or is it intended for humans?

No, this will not be in the final patch.  Sorry, I thought the comment was clear enough.  This was my way of keeping track of when I really considered each test to be "verified" that it test what we think its testing.  As I've been verifying tests, I've been migrating them to LayerChromiumTest, and this WORK_IN_PROGRESS class will be deleted.

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:308
>> +    //        for now, this will print warnings in the test but not fail.
> 
> Does it matter how many times notifySyncRequired is called?
> 
> Tests should be silent when passing.

I feel like it does indeed matter how many times notifySyncRequired is called.  If we're planning to make LayerChromium part of a "public CC API", where the user has to receive these notifySyncRequired calls, we should clearly define when/why it would be called.  To me, it makes sense if this is called exactly once per public function, or not at all, but definitely not multiple times.  The way the code is now, whenever we remove all children, or even in the destructor which feels particularly weird, notifySyncRequired can get called several times in a row.   It doesn't seem like a good idea to have that sort of behavior on the public api side of the code... thoughts?

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:358
>> +TEST_F(LayerChromiumTest_WORK_IN_PROGRESS, replaceChildWithNewChild)
> 
> these tests raise an interesting point: why is replaceChild part of our interface anyway? I can't find any callers, and it looks like it was added in the initial import of the GraphicsLayer stuff.  I'll follow up with Simon to see if we need to keep this.

OK, I'll adapt this code to whatever you all decide.

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:507
>> +    // and override that function so that we can test descendantDrawsContent().
> 
> It's not a good idea to check in intentionally disabled code.  Code that is in the repo is code that the whole project has to maintain, even if it's disabled or dead code.  If you aren't sure whether to land something then keep it in a local git repo or in a patch on another bug.

Yes, I didn't intend to land any disabled tests.  the point was for us to discuss the LayerChromium semantics.  I believe that we're discussing this right now in https://bugs.webkit.org/show_bug.cgi?id=67750 and I would prefer to settle that issue before we land this patch.  So yeah, this will either be enabled, or removed entirely, before landing.

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:584
>> +    /*
> 
> why is this part commented out?

my mistake, I should uncomment and fix this code.

>> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:658
>> +    Mock::VerifyAndClearExpectations(&mockDelegate);
> 
> we repeat this stanza a lot, seems like it could use a helper function of some sort

OK, sure.  But I think it would require a macro because of the arbitrary function in the middle (including arbitrary parameters, so templates don't seem practical either).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.