[Webkit-unassigned] [Bug 68572] Create unit tests for LayerChromium
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Sep 22 16:46:31 PDT 2011
https://bugs.webkit.org/show_bug.cgi?id=68572
James Robinson <jamesr at chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #108272|review? |review-
Flag| |
--- Comment #5 from James Robinson <jamesr at chromium.org> 2011-09-22 16:46:31 PST ---
(From update of attachment 108272)
View in context: https://bugs.webkit.org/attachment.cgi?id=108272&action=review
First round of comments. This definitely needs some work, but I'm really excited about having more coverage here.
> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:34
> +using ::testing::_;
what is this?
> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:67
> + static int m_numInstancesDestroyed;
we tend to use an s_ prefix for statics (as opposed to an m_ prefix for member variables) so this should be s_numInstancesDestroyed
> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:74
> + void SetUp()
this function is virtual (since testing::Test::SetUp() is declared virtual in gtest.h). Please include the virtual keyword here
> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:83
> + // silentDelegate is initialized to be just a stub and will
> + // not print any warnings. It is used when we are not worried
> + // about testing how the delegate is called.
> +
> + EXPECT_CALL(silentDelegate, drawsContent()).Times(AnyNumber());
> + EXPECT_CALL(silentDelegate, preserves3D()).Times(AnyNumber());
> + EXPECT_CALL(silentDelegate, paintContents(_, _)).Times(AnyNumber());
> + EXPECT_CALL(silentDelegate, notifySyncRequired()).Times(AnyNumber());
isn't this exactly the same as passing 0 in for the delegate?
> 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?
> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:156
> + // All delegates and layers are declared here so that each test case
> + // is more readable. The meaning behind variable names should be
> + // clear when reading the specific test cases.
> + MockLayerDelegate mockDelegate, silentDelegate, parentDelegate, childDelegate;
> + 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.
> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:160
> +// FIXME: this fixture is temporary just while developing test cases
> +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?
> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:162
> + void SetUp() { LayerChromiumTest::SetUp(); }
this line is a complete no-op. LayerChromiumTest::SetUp() is virtual (because void SetUp() is declared virtual in testing::Test()), so if you left this out calling SetUp() on an instance of LayerChromiumTest_WORK_IN_PROGRESS would call LayerChromiumTest::SetUp()
> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:163
> + void createSimpleTestTree() { LayerChromiumTest::createSimpleTestTree(); }
why is this here?
> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:188
> + EXPECT_EQ(parent->children().size(), (size_t)0);
We use C++ style casts, not C style. That means this should be static_cast<size_t>(0)
> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:193
> + EXPECT_EQ(parent->children().size(), (size_t)1);
should be static_cast<>. there are many more instances of this in the rest of the code, so I didn't mark them all.
> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:275
> + // insert to empty list
> + EXPECT_CALL(parentDelegate, notifySyncRequired()).Times(1);
> + parent->insertChild(child3, 0);
> + Mock::VerifyAndClearExpectations(&parentDelegate);
It's not useful to include a comment that merely repeats what the next line of code does. We can all read the code. In a comment you should try to provide information that is not obvious from just reading the code - for instance, explain why you are doing things in a certain way, subtle interactions between systems that aren't clear from just reading the code, etc. If there aren't any such things to note, then just don't add a comment.
> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:308
> + // FIXME: There are awkward semantics that notifySyncRequired gets
> + // called for each child that gets removed from within the
> + // parent's destructor. are those semantics OK? It feels
> + // like it may come back to bite us down the road.
> + // 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.
> 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.
> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:507
> + // FIXME: the base class LayerChromium always returns false and doesn't use the delegate drawsContent().
> + // this test should be disabled until we resolve whether those semantics are appropriate or not.
> + // if it should return false without calling delegate, then we have to subclass the LayerChromium
> + // 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.
> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:584
> + /*
why is this part commented out?
> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:658
> + EXPECT_CALL(mockDelegate, notifySyncRequired()).Times(1);
> + testLayer->setAnchorPoint(FloatPoint(1.23f, 4.56f));
> + Mock::VerifyAndClearExpectations(&mockDelegate);
we repeat this stanza a lot, seems like it could use a helper function of some sort
> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:730
> + // FIXME: uncomment this when we can reasonably stub the CCLayerTreeHost.
> + //
> + // EXPECT_CALL(mockDelegate, notifySyncRequired()).Times(1);
> + // testLayer->setLayerTreeHost(dummyPointer);
> + // Mock::VerifyAndClearExpectations(&mockDelegate);
> + // verifyFloatRectsAlmostEqual(testLayer->dirtyRect(), FloatRect(0.0f, 0.0f, 5.0f, 10.0f));
> + //
> + // testLayer->resetNeedsDisplay();
> + // EXPECT_TRUE(testLayer->dirtyRect().isEmpty());
see commments above about not checking in commented out code
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list