Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1517463003:
Added comprehensive tests for views::Border. (Closed)
|
Created:
5 years ago by Matt Giuca Modified:
4 years, 6 months ago CC:
chromium-reviews, tfarina, danakj, Ben Goodger (Google), chrome-apps-syd-reviews_chromium.org, calamity Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded comprehensive tests for views::Border.
BUG=568389
Committed: https://crrev.com/92614dc154ad143a1ef9849ae0fe90c02df09f30
Committed: https://crrev.com/23a6dcc694a5d93d678c203dc834fb1b9dbd9fbf
Cr-Original-Commit-Position: refs/heads/master@{#399650}
Cr-Commit-Position: refs/heads/master@{#399852}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove use of evil GMock and instead hand-roll custom mock classes. #
Total comments: 4
Patch Set 3 : Respond to danakj review. #Patch Set 4 : DISALLOW_COPY_AND_ASSIGN. #Patch Set 5 : Re-enable test: I am landing the fix before this CL so it doesn't need to be disabled. #Patch Set 6 : Rebase. #Patch Set 7 : Rebase. #Patch Set 8 : Rebase. #
Total comments: 10
Patch Set 9 : Rebase. #Patch Set 10 : Fix compile errors due to upstream changes. #Patch Set 11 : Respond to sadrul review. #Patch Set 12 : Rebase. #
Total comments: 6
Patch Set 13 : Added note. #Messages
Total messages: 51 (15 generated)
Description was changed from ========== Added comprehensive tests for views::Border. BorderTest.RoundedRectBorder is disabled due to http://crbug.com/565801. That will be fixed and enabled in a follow-up CL. Also adds GoogleMock dependencies back into ui/views. BUG=568389 ========== to ========== Added comprehensive tests for views::Border. BorderTest.RoundedRectBorder is disabled due to http://crbug.com/565801. That will be fixed and enabled in a follow-up CL. Also adds GoogleMock dependencies back into ui/views. BUG=568389, 134675 ==========
mgiuca@chromium.org changed reviewers: + sadrul@chromium.org
+tfarina, danakj and ben FYI about allowing GMock in ui/views. As I said on the bug, I don't know why GMock is banned (is there a legitimate engineering reason or just some historical artifact?). But it is very helpful so I would prefer to unban it rather than rewrite this test to manually mock everything.
drive-by: FWIW I don't like gmock. The tests are hard to read (i.e. both the code, and the inevitable failures). Simple API changes can have trickle-down effects on a whole bunch of unrelated tests; gmock tests have additional maintenance burden compared tests that do not use gmock. They can encourage APIs to be unnecessarily "virtual" or have additional/protected accessors for no good reason, just so a response can be mocked. I also often find that the kind of tests written with gmock are "uninteresting" - they test "am I calling this function now" rather than "does this call have the effect I want". Compare this to a friend TestApi class, which is less intrusive and is able to provide access to read changes in private state due to the _real_ function call rather than a mocked out one.
On 2015/12/10 02:42:15, tapted wrote: > drive-by: FWIW I don't like gmock. The tests are hard to read (i.e. both the > code, and the inevitable failures). Simple API changes can have trickle-down > effects on a whole bunch of unrelated tests; gmock tests have additional > maintenance burden compared tests that do not use gmock. > > They can encourage APIs to be unnecessarily "virtual" or have > additional/protected accessors for no good reason, just so a response can be > mocked. > > I also often find that the kind of tests written with gmock are "uninteresting" > - they test "am I calling this function now" rather than "does this call have > the effect I want". Compare this to a friend TestApi class, which is less > intrusive and is able to provide access to read changes in private state due to > the _real_ function call rather than a mocked out one. Yeah, we don't allow gmock in views, aura etc. The exclusion from DEPS is deliberate. There was a thread about it on chromium-dev@ a while back. If should be easy to replace gmock from this patch. So not lgtm
On 2015/12/10 02:50:13, sadrul wrote: > On 2015/12/10 02:42:15, tapted wrote: > > drive-by: FWIW I don't like gmock. The tests are hard to read (i.e. both the > > code, and the inevitable failures). Simple API changes can have trickle-down > > effects on a whole bunch of unrelated tests; gmock tests have additional > > maintenance burden compared tests that do not use gmock. > > > > They can encourage APIs to be unnecessarily "virtual" or have > > additional/protected accessors for no good reason, just so a response can be > > mocked. > > > > I also often find that the kind of tests written with gmock are > "uninteresting" > > - they test "am I calling this function now" rather than "does this call have > > the effect I want". Compare this to a friend TestApi class, which is less > > intrusive and is able to provide access to read changes in private state due > to > > the _real_ function call rather than a mocked out one. > > Yeah, we don't allow gmock in views, aura etc. The exclusion from DEPS is > deliberate. There was a thread about it on chromium-dev@ a while back. If should > be easy to replace gmock from this patch. Any idea what it's called? I can't find it from a simple search. > > So not lgtm
On 2015/12/10 03:32:57, Matt Giuca wrote: > On 2015/12/10 02:50:13, sadrul wrote: > > On 2015/12/10 02:42:15, tapted wrote: > > > drive-by: FWIW I don't like gmock. The tests are hard to read (i.e. both the > > > code, and the inevitable failures). Simple API changes can have trickle-down > > > effects on a whole bunch of unrelated tests; gmock tests have additional > > > maintenance burden compared tests that do not use gmock. > > > > > > They can encourage APIs to be unnecessarily "virtual" or have > > > additional/protected accessors for no good reason, just so a response can be > > > mocked. > > > > > > I also often find that the kind of tests written with gmock are > > "uninteresting" > > > - they test "am I calling this function now" rather than "does this call > have > > > the effect I want". Compare this to a friend TestApi class, which is less > > > intrusive and is able to provide access to read changes in private state due > > to > > > the _real_ function call rather than a mocked out one. > > > > Yeah, we don't allow gmock in views, aura etc. The exclusion from DEPS is > > deliberate. There was a thread about it on chromium-dev@ a while back. If > should > > be easy to replace gmock from this patch. > > Any idea what it's called? I can't find it from a simple search. I guess it's this: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/-KH_IP0rIWQ/dis... It seems like a decision was never made, and various owners said they either would or wouldn't ban it. No consensus was reached. That is not how we make high-level decisions on Chromium. We don't ban certain C++11 features (for example) in one directory but not another, on the preferences of the owners. Is GMock banned in Chromium or is it not? Looks like there are two arguments: Trent is saying not to use mocks at all, because they are brittle. Sadrul is saying not to use GMock, but instead to roll my own mocking class. Re brittle mocks: the only alternative here is writing a pixel test. I am trying to shield the Border code from the enormous implementation complexities of Canvas and Skia. This is just basic unit testing practice: you don't test an entire stack of APIs, you test one API and make sure it is behaving correctly. I could do a pixel test but then a slight change to anti-aliased Bezier curve rendering in Skia would break the test. Re GMock vs rolling your own: I can easily change this code to make my own mock classes instead. It seems pretty dumb to do that when we have a nice library that does it for us. Doing so won't change how brittle this test is (although it will probably make the failure output neater). It will mostly just be adding a bunch of extra code to satisfy an arbitrary rule about using GMock in a certain directory. > > > > > So not lgtm
Description was changed from ========== Added comprehensive tests for views::Border. BorderTest.RoundedRectBorder is disabled due to http://crbug.com/565801. That will be fixed and enabled in a follow-up CL. Also adds GoogleMock dependencies back into ui/views. BUG=568389, 134675 ========== to ========== Added comprehensive tests for views::Border. BorderTest.RoundedRectBorder is disabled due to http://crbug.com/565801. That will be fixed and enabled in a follow-up CL. BUG=568389, 134675 ==========
PTAL. For the record, I am strongly opposed to the changes of Patch Set 2. Why write all this code by hand when you have a library available and used elsewhere in Chromium that is designed for this specific purpose? So when someone comes by this CL and says "why didn't he use GMock?", let it be known that I tried.
danakj@chromium.org changed reviewers: + danakj@chromium.org
PS1 looks good. It tests that the border code calls the right things on Canvas. That is the point of the border code, so that seems like the right level of testing. PS2 is near unreadable. I'm not sure but maybe you could work on making it more clear, hopefully it's not meant to be a strawman :) I think gmock is very useful when you want to test that something is called. Many tests don't want to test that, they want to check state on objects, and TestAPI is good for testing state changes. gmock is good for testing interactions. https://codereview.chromium.org/1517463003/diff/1/ui/views/border_unittest.cc File ui/views/border_unittest.cc (right): https://codereview.chromium.org/1517463003/diff/1/ui/views/border_unittest.cc... ui/views/border_unittest.cc:51: canvas_.reset(new gfx::Canvas(sk_canvas_.get(), 1.0f)); Can you make a TearDown() method that cleans this up in reverse order? https://codereview.chromium.org/1517463003/diff/20001/ui/views/border_unittes... File ui/views/border_unittest.cc (right): https://codereview.chromium.org/1517463003/diff/20001/ui/views/border_unittes... ui/views/border_unittest.cc:127: auto draw_rect_calls = sk_canvas_->draw_rect_calls(); const auto& please always qualify auto as much as possible https://codereview.chromium.org/1517463003/diff/20001/ui/views/border_unittes... ui/views/border_unittest.cc:152: auto draw_rrect_calls = sk_canvas_->draw_rrect_calls(); const auto&
On 2015/12/10 20:01:27, danakj (behind on reviews) wrote: > PS2 is near unreadable. I'm not sure but maybe you could work on making it more > clear, hopefully it's not meant to be a strawman :) It's somewhat of a strawman in that I explicitly think it's the wrong thing to do, but I didn't deliberately make it "bad". I was following the examples of other CLs that removed GMock and replaced it with custom mock classes: https://codereview.chromium.org/56193006 https://codereview.chromium.org/107363004 I'm not really sure how to improve it. I need to record the arguments that came into the call and later compare them (can't just record counts or anything like that). > I think gmock is very useful when you want to test that something is called. > Many tests don't want to test that, they want to check state on objects, and > TestAPI is good for testing state changes. gmock is good for testing > interactions. Are you suggesting that I can change away from using mocks? (I don't know what TestAPI is and searching in code search just shows a bunch of classes called TestAPI.) I think we want to test interactions here. (The "state" is pixel data and pixel testing is a painful flaky path.) > > https://codereview.chromium.org/1517463003/diff/1/ui/views/border_unittest.cc > File ui/views/border_unittest.cc (right): > > https://codereview.chromium.org/1517463003/diff/1/ui/views/border_unittest.cc... > ui/views/border_unittest.cc:51: canvas_.reset(new gfx::Canvas(sk_canvas_.get(), > 1.0f)); > Can you make a TearDown() method that cleans this up in reverse order? > > https://codereview.chromium.org/1517463003/diff/20001/ui/views/border_unittes... > File ui/views/border_unittest.cc (right): > > https://codereview.chromium.org/1517463003/diff/20001/ui/views/border_unittes... > ui/views/border_unittest.cc:127: auto draw_rect_calls = > sk_canvas_->draw_rect_calls(); > const auto& > > please always qualify auto as much as possible > > https://codereview.chromium.org/1517463003/diff/20001/ui/views/border_unittes... > ui/views/border_unittest.cc:152: auto draw_rrect_calls = > sk_canvas_->draw_rrect_calls(); > const auto& https://codereview.chromium.org/1517463003/diff/1/ui/views/border_unittest.cc File ui/views/border_unittest.cc (right): https://codereview.chromium.org/1517463003/diff/1/ui/views/border_unittest.cc... ui/views/border_unittest.cc:51: canvas_.reset(new gfx::Canvas(sk_canvas_.get(), 1.0f)); On 2015/12/10 20:01:27, danakj (behind on reviews) wrote: > Can you make a TearDown() method that cleans this up in reverse order? Done. Thanks, I forgot that these would not be automatically cleaned up in reverse order (unlike a normal class automatic constructor/destructor). https://codereview.chromium.org/1517463003/diff/20001/ui/views/border_unittes... File ui/views/border_unittest.cc (right): https://codereview.chromium.org/1517463003/diff/20001/ui/views/border_unittes... ui/views/border_unittest.cc:127: auto draw_rect_calls = sk_canvas_->draw_rect_calls(); On 2015/12/10 20:01:27, danakj (behind on reviews) wrote: > const auto& > > please always qualify auto as much as possible Done. https://codereview.chromium.org/1517463003/diff/20001/ui/views/border_unittes... ui/views/border_unittest.cc:152: auto draw_rrect_calls = sk_canvas_->draw_rrect_calls(); On 2015/12/10 20:01:27, danakj (behind on reviews) wrote: > const auto& Done.
On Thu, Dec 10, 2015 at 4:03 PM, <mgiuca@chromium.org> wrote: > On 2015/12/10 20:01:27, danakj (behind on reviews) wrote: > >> PS2 is near unreadable. I'm not sure but maybe you could work on making it >> > more > >> clear, hopefully it's not meant to be a strawman :) >> > > It's somewhat of a strawman in that I explicitly think it's the wrong > thing to > do, but I didn't deliberately make it "bad". I was following the examples > of > other CLs that removed GMock and replaced it with custom mock classes: > https://codereview.chromium.org/56193006 > https://codereview.chromium.org/107363004 > > I'm not really sure how to improve it. I need to record the arguments that > came > into the call and later compare them (can't just record counts or anything > like > that). > > I think gmock is very useful when you want to test that something is >> called. >> Many tests don't want to test that, they want to check state on objects, >> and >> TestAPI is good for testing state changes. gmock is good for testing >> interactions. >> > > Are you suggesting that I can change away from using mocks? (I don't know > what > TestAPI is and searching in code search just shows a bunch of classes > called > TestAPI.) I think we want to test interactions here. (The "state" is pixel > data > and pixel testing is a painful flaky path.) *TestAPI classes are a pattern where Foo friends FooTestAPI which provides access to private state for checking state changes in tests. I don't think it applies here though is what I meant to say. I think you want to test for calls on a canvas (or test state on the resulting bitmap). Writing your own mocks is kinda awful, so I think gmock makes a lot of sense. If views owners really hate it for some reason, my personal preference is probably pixel-testing a resulting bitmap. But then verifying and fixing test failures is pretty sad instead (as you also point out) so I peraonally prefer mocks for this. Throwing my 2c in here cuz I was on the CL but I'm also not an owner here, so take it as you will. I do think it's okay for components to set rules for what can appear in their subdirectory as long as it's being more strict, not less, than chromium generally allows (such as banning gmock). > > > > >> https://codereview.chromium.org/1517463003/diff/1/ui/views/border_unittest.cc >> File ui/views/border_unittest.cc (right): >> > > > > https://codereview.chromium.org/1517463003/diff/1/ui/views/border_unittest.cc... > >> ui/views/border_unittest.cc:51: canvas_.reset(new >> > gfx::Canvas(sk_canvas_.get(), > >> 1.0f)); >> Can you make a TearDown() method that cleans this up in reverse order? >> > > > > https://codereview.chromium.org/1517463003/diff/20001/ui/views/border_unittes... > >> File ui/views/border_unittest.cc (right): >> > > > > https://codereview.chromium.org/1517463003/diff/20001/ui/views/border_unittes... > >> ui/views/border_unittest.cc:127: auto draw_rect_calls = >> sk_canvas_->draw_rect_calls(); >> const auto& >> > > please always qualify auto as much as possible >> > > > > https://codereview.chromium.org/1517463003/diff/20001/ui/views/border_unittes... > >> ui/views/border_unittest.cc:152: auto draw_rrect_calls = >> sk_canvas_->draw_rrect_calls(); >> const auto& >> > > > > > https://codereview.chromium.org/1517463003/diff/1/ui/views/border_unittest.cc > File ui/views/border_unittest.cc (right): > > > https://codereview.chromium.org/1517463003/diff/1/ui/views/border_unittest.cc... > ui/views/border_unittest.cc:51: canvas_.reset(new > gfx::Canvas(sk_canvas_.get(), 1.0f)); > On 2015/12/10 20:01:27, danakj (behind on reviews) wrote: > >> Can you make a TearDown() method that cleans this up in reverse order? >> > > Done. > > Thanks, I forgot that these would not be automatically cleaned up in > reverse order (unlike a normal class automatic constructor/destructor). > > > https://codereview.chromium.org/1517463003/diff/20001/ui/views/border_unittes... > File ui/views/border_unittest.cc (right): > > > https://codereview.chromium.org/1517463003/diff/20001/ui/views/border_unittes... > ui/views/border_unittest.cc:127: auto draw_rect_calls = > sk_canvas_->draw_rect_calls(); > On 2015/12/10 20:01:27, danakj (behind on reviews) wrote: > >> const auto& >> > > please always qualify auto as much as possible >> > > Done. > > > https://codereview.chromium.org/1517463003/diff/20001/ui/views/border_unittes... > ui/views/border_unittest.cc:152: auto draw_rrect_calls = > sk_canvas_->draw_rrect_calls(); > On 2015/12/10 20:01:27, danakj (behind on reviews) wrote: > >> const auto& >> > > Done. > > https://codereview.chromium.org/1517463003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1517463003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1517463003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #7 (id:120001) has been deleted
Hi Sadrul, Can you comment on this? I wrote a version with gmock, and you said to replace it with a non-gmock version. So I did that. Both Dana and I think it's hideous but I'd like to get your opinion on how this should proceed. I'm unclear about whether you like gmock but rules are rules, whether you dislike gmock but are OK with mocking in general, or whether you dislike mocking altogether. a) Allow gmock (PS1). b) Use the non-gmock version (PS7). c) Avoid mocking and test some other way (pixel tests). d) Just leave it untested. TBH this is a side track for me and I've already (re)written it twice. I don't think it's worth my time to write these tests again for (c). And I don't think maintaining pixel tests is the right thing to do here when it is so easy and correct to verify the graphics calls.
> Hi Sadrul, > > Can you comment on this? I wrote a version with gmock, and you said to replace > it with a non-gmock version. So I did that. Both Dana and I think it's hideous > but I'd like to get your opinion on how this should proceed. I'm unclear about > whether you like gmock but rules are rules, whether you dislike gmock but are OK > with mocking in general, or whether you dislike mocking altogether. Yes. (... sorry, had to) I am not a fan of gmock. I find gmock tests more difficult to read, and I am glad I don't have to deal with them too often. (maybe these are related) > > a) Allow gmock (PS1). > b) Use the non-gmock version (PS7). > c) Avoid mocking and test some other way (pixel tests). > d) Just leave it untested. > > TBH this is a side track for me and I've already (re)written it twice. I don't > think it's worth my time to write these tests again for (c). And I don't think > maintaining pixel tests is the right thing to do here when it is so easy and > correct to verify the graphics calls. I would go with tests avoiding gmock. But looking at the tests in this CL, I think there are some unnecessary stuff in there (e.g. what sort of regression do you expect to catch with checking the min-size or the insets of the borders?). Testing that a blue-border is really painting blue also doesn't seem useful (it could make sense if the same SolidSidedBorder object juggled with multiple colors based on some conditions, but that is not the case). Testing that the right set of calls are made on the SkCanvas in the right order may be useful (although it is pretty simple for the current borders). I am not a fan of pixel tests either. It'd be pretty neat if skia already something we could use instead (e.g. would SkPictureRecord or something like that be useful here? or maybe blink has something?)
On Fri, Jan 8, 2016 at 1:27 PM, <sadrul@chromium.org> wrote: > Hi Sadrul, >> > > Can you comment on this? I wrote a version with gmock, and you said to >> replace >> it with a non-gmock version. So I did that. Both Dana and I think it's >> hideous >> but I'd like to get your opinion on how this should proceed. I'm unclear >> about >> whether you like gmock but rules are rules, whether you dislike gmock but >> are >> > OK > >> with mocking in general, or whether you dislike mocking altogether. >> > > Yes. > > (... sorry, had to) > > I am not a fan of gmock. I find gmock tests more difficult to read, and I > am > glad I don't have to deal with them too often. (maybe these are related) > > > a) Allow gmock (PS1). >> b) Use the non-gmock version (PS7). >> c) Avoid mocking and test some other way (pixel tests). >> d) Just leave it untested. >> > > TBH this is a side track for me and I've already (re)written it twice. I >> don't >> think it's worth my time to write these tests again for (c). And I don't >> think >> maintaining pixel tests is the right thing to do here when it is so easy >> and >> correct to verify the graphics calls. >> > > I would go with tests avoiding gmock. > > But looking at the tests in this CL, I think there are some unnecessary > stuff in > there (e.g. what sort of regression do you expect to catch with checking > the > min-size or the insets of the borders?). Testing that a blue-border is > really > painting blue also doesn't seem useful (it could make sense if the same > SolidSidedBorder object juggled with multiple colors based on some > conditions, > but that is not the case). Testing that the right set of calls are made on > the > SkCanvas in the right order may be useful (although it is pretty simple > for the > current borders). I am not a fan of pixel tests either. It'd be pretty > neat if > skia already something we could use instead (e.g. would SkPictureRecord or > something like that be useful here? or maybe blink has something?) > Blink uses layout tests (pixel tests) for checking paint outputs. I don't think there are unit tests that do the same thing right now? cc uses pixel tests or gmock (usually for verifying openGL calls instead of canvas but same idea, and we avoid pixel tests when possible). You could record into a picture but when it fails what will the output be to debug? I don't think they're well suited to being used as test expections atm. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/01/08 21:27:54, sadrul wrote: > > Hi Sadrul, > > > > Can you comment on this? I wrote a version with gmock, and you said to replace > > it with a non-gmock version. So I did that. Both Dana and I think it's hideous > > but I'd like to get your opinion on how this should proceed. I'm unclear about > > whether you like gmock but rules are rules, whether you dislike gmock but are > OK > > with mocking in general, or whether you dislike mocking altogether. > > Yes. > > (... sorry, had to) > > I am not a fan of gmock. I find gmock tests more difficult to read, and I am > glad I don't have to deal with them too often. (maybe these are related) I think that my hand-rolled mocks (or any hand-rolled mocks) are much harder to read than GMock --- compare PS1 to PS7. Once you are used to GMock, you get familiar with the idioms and then it becomes effortless to read, compared to some giant recording and playback system. This is sort of like saying "I find it hard to read C++ so I wrote my own programming language." C++ is difficult to read if you haven't seen it before but we still use it because once we learn it it provides us with useful abstractions that many many clever people have built over many many years. I feel the same way about GMock. > > > > > a) Allow gmock (PS1). > > b) Use the non-gmock version (PS7). > > c) Avoid mocking and test some other way (pixel tests). > > d) Just leave it untested. > > > > TBH this is a side track for me and I've already (re)written it twice. I don't > > think it's worth my time to write these tests again for (c). And I don't think > > maintaining pixel tests is the right thing to do here when it is so easy and > > correct to verify the graphics calls. > > I would go with tests avoiding gmock. So b / PS7, or something else avoiding mocking altogether? > But looking at the tests in this CL, I think there are some unnecessary stuff in > there (e.g. what sort of regression do you expect to catch with checking the > min-size or the insets of the borders?). Any regression to those methods. I don't write tests intending to catch specific regressions. There are methods that are supposed to return a particular value, so I call them in the test and check that the result equals the expected value. > Testing that a blue-border is really > painting blue also doesn't seem useful (it could make sense if the same > SolidSidedBorder object juggled with multiple colors based on some conditions, > but that is not the case). What do you mean "doesn't seem useful"? If you look at tests and go "I can't see why these would fail, let's take them out" then there isn't much point having tests at all. We want to comprehensively ensure that this thing is behaving the way it should. And that means when I ask SolidSidedBorder to draw blue, that it draws four blue lines. I need to check that each line being painted is blue. > Testing that the right set of calls are made on the > SkCanvas in the right order may be useful (although it is pretty simple for the > current borders). Actually, we ideally *do not* want to care about the order that the calls are made (because it is irrelevant for the SolidSidedBorder). In my original GMock tests, I was careful not to enforce the order. In my own hand-rolled tests, it was too hard to do that so I just enforce a specific order. (Yet another reason to use GMock --- it has lots of useful features that are too hard to do if everybody has to write their own mocking by hand.) > It'd be pretty neat if > skia already something we could use instead (e.g. would SkPictureRecord or > something like that be useful here? or maybe blink has something?) I originally found some tests in Blink that were hand-rolling their own recording system (I don't remember exactly where, but third_party/WebKit/Source/core/page/PrintContextTest.cpp does this). "Eww", I thought, "I can just use Gmock." Didn't realise GMock was shunned. There's also a bunch of tests in Blink that directly use GMock: third_party/WebKit/Source/web/PageOverlayTest.cpp third_party/WebKit/Source/platform/graphics/paint/PaintArtifactToSkCanvasTest.cpp Any system that records commands and then you verify that the recorded commands matched your expectations is essentially the same as a mock. It has the same shortcomings as using GMock (brittle tests that depend somewhat on the implementation details). So if we're going to use this approach, we may as well use the standard syntax and abstractions of GMock.
Hi Sadrul, This has been sitting around for months. Are you able to comment on my previous email or should we just drop this CL? I spent a lot of effort writing a comprehensive test suite for Border (twice, in fact: one with gmock and one with a hand-rolled mock class). It would be a shame to leave it untested just because of arbitrary rules about which bits of code are allowed to use Gmock / mocking. But if this isn't going to land I may as well close it.
On 2016/01/11 00:00:00, Matt Giuca wrote: > On 2016/01/08 21:27:54, sadrul wrote: > > > Hi Sadrul, > > > > > > Can you comment on this? I wrote a version with gmock, and you said to > replace > > > it with a non-gmock version. So I did that. Both Dana and I think it's > hideous > > > but I'd like to get your opinion on how this should proceed. I'm unclear > about > > > whether you like gmock but rules are rules, whether you dislike gmock but > are > > OK > > > with mocking in general, or whether you dislike mocking altogether. > > > > Yes. > > > > (... sorry, had to) > > > > I am not a fan of gmock. I find gmock tests more difficult to read, and I am > > glad I don't have to deal with them too often. (maybe these are related) > > I think that my hand-rolled mocks (or any hand-rolled mocks) are much harder to > read than GMock --- compare PS1 to PS7. Once you are used to GMock, you get > familiar with the idioms and then it becomes effortless to read, compared to > some giant recording and playback system. This is sort of like saying "I find it > hard to read C++ so I wrote my own programming language." C++ is difficult to > read if you haven't seen it before but we still use it because once we learn it > it provides us with useful abstractions that many many clever people have built > over many many years. I feel the same way about GMock. > > > > > > > > > a) Allow gmock (PS1). > > > b) Use the non-gmock version (PS7). > > > c) Avoid mocking and test some other way (pixel tests). > > > d) Just leave it untested. > > > > > > TBH this is a side track for me and I've already (re)written it twice. I > don't > > > think it's worth my time to write these tests again for (c). And I don't > think > > > maintaining pixel tests is the right thing to do here when it is so easy and > > > correct to verify the graphics calls. > > > > I would go with tests avoiding gmock. > > So b / PS7, or something else avoiding mocking altogether? > > > But looking at the tests in this CL, I think there are some unnecessary stuff > in > > there (e.g. what sort of regression do you expect to catch with checking the > > min-size or the insets of the borders?). > > Any regression to those methods. I don't write tests intending to catch specific > regressions. There are methods that are supposed to return a particular value, > so I call them in the test and check that the result equals the expected value. These are essentially change-detector tests, and we shouldn't have them (see tott episode 350). > > > Testing that a blue-border is really > > painting blue also doesn't seem useful (it could make sense if the same > > SolidSidedBorder object juggled with multiple colors based on some conditions, > > but that is not the case). > > What do you mean "doesn't seem useful"? If you look at tests and go "I can't see > why these would fail, let's take them out" then there isn't much point having > tests at all. We want to comprehensively ensure that this thing is behaving the > way it should. And that means when I ask SolidSidedBorder to draw blue, that it > draws four blue lines. I need to check that each line being painted is blue. > > > Testing that the right set of calls are made on the > > SkCanvas in the right order may be useful (although it is pretty simple for > the > > current borders). > > Actually, we ideally *do not* want to care about the order that the calls are > made (because it is irrelevant for the SolidSidedBorder). In my original GMock > tests, I was careful not to enforce the order. In my own hand-rolled tests, it > was too hard to do that so I just enforce a specific order. (Yet another reason > to use GMock --- it has lots of useful features that are too hard to do if > everybody has to write their own mocking by hand.) > > > It'd be pretty neat if > > skia already something we could use instead (e.g. would SkPictureRecord or > > something like that be useful here? or maybe blink has something?) > > I originally found some tests in Blink that were hand-rolling their own > recording system (I don't remember exactly where, but > third_party/WebKit/Source/core/page/PrintContextTest.cpp does this). "Eww", I > thought, "I can just use Gmock." Didn't realise GMock was shunned. > > There's also a bunch of tests in Blink that directly use GMock: > > third_party/WebKit/Source/web/PageOverlayTest.cpp > third_party/WebKit/Source/platform/graphics/paint/PaintArtifactToSkCanvasTest.cpp > > Any system that records commands and then you verify that the recorded commands > matched your expectations is essentially the same as a mock. It has the same > shortcomings as using GMock (brittle tests that depend somewhat on the > implementation details). So if we're going to use this approach, we may as well > use the standard syntax and abstractions of GMock.
On 2016/05/24 20:29:09, sadrul wrote: > > Any regression to those methods. I don't write tests intending to catch > specific > > regressions. There are methods that are supposed to return a particular value, > > so I call them in the test and check that the result equals the expected > value. > > These are essentially change-detector tests, and we shouldn't have them (see > tott episode 350). What? These are perfectly normal (albeit simple) unit tests. The normal pattern for unit testing a class is: 1. Create instance of class. 2. Call all of the class's getter methods and ensure that they return the expected values. In this specific case: 136 scoped_ptr<Border> border(Border::CreateSolidBorder(3, SK_ColorBLUE)); 137 EXPECT_EQ(gfx::Size(6, 6), border->GetMinimumSize()); 138 EXPECT_EQ(gfx::Insets(3, 3, 3, 3), border->GetInsets()); That TOTT is using the term "change detectors" for tests that care too much about the implementation details and not about the results. I am doing the exact opposite: I am inspecting the public accessors of the class to make sure that the class did *something* (anything) to produce the correct values. I don't care how it did it, for example how the size and insets are being stored internally. I wrote a test that checks that when I instantiate a class using its public interface that its public accessors function correctly. This is textbook black-box testing. In the CreateSolidBorder case, actually a fair amount of logic is going on in the implementation. It takes a single int 3, calls gfx::Insets(int all) which duplicates the int four times into the top, bottom, left and right fields. It then stores those insets internally, and returns them if I call GetInsets(). GetMinimumSize() calls width() which sums the left and right insets, producing 6, and same for height(). My test doesn't care about any of those implementation details; for example, the SolidSidedBorder could be modified to just store "int inset_ = 3". Then GetInsets() would return gfx::Insets(inset_), resulting in Insets(3, 3, 3, 3). And GetMinimumSize() could just return gfx::Size(inset_ * 2, inset_ * 2). My test would still pass, just as long as the implementation produces the correct results. And those are useful tests, because for example if GetMinimumSize were to return just the left and top (forgetting to sum the left+right, and top+bottom), it would return gfx::Size(3, 3) and fail the test. Now that small nit aside, you still have not yet at all addressed the basic question I asked on Monday which is: do you want me to abandon this CL, or can I land it? If you are not happy with it in its current form, please give specific actionable changes you would like me to make. As I have said, I would much prefer to use Gmock which is a library specifically designed for writing this sort of test, rather than hand-writing my own mocking system just for this one file, and Dana has expressed this preference too. But I would rather land the non-Gmock one than have no tests at all. If you don't like the very idea of recording the draw commands and ensuring they match expectations, then this CL is not landable, so please say so and I will close it.
On 2016/05/26 07:01:31, Matt Giuca wrote: > On 2016/05/24 20:29:09, sadrul wrote: > > > Any regression to those methods. I don't write tests intending to catch > > specific > > > regressions. There are methods that are supposed to return a particular > value, > > > so I call them in the test and check that the result equals the expected > > value. > > > > These are essentially change-detector tests, and we shouldn't have them (see > > tott episode 350). > > What? These are perfectly normal (albeit simple) unit tests. The normal pattern > for unit testing a class is: > > 1. Create instance of class. > 2. Call all of the class's getter methods and ensure that they return the > expected values. > > In this specific case: > > 136 scoped_ptr<Border> border(Border::CreateSolidBorder(3, SK_ColorBLUE)); > 137 EXPECT_EQ(gfx::Size(6, 6), border->GetMinimumSize()); > 138 EXPECT_EQ(gfx::Insets(3, 3, 3, 3), border->GetInsets()); > > That TOTT is using the term "change detectors" for tests that care too much > about the implementation details and not about the results. I am doing the exact > opposite: I am inspecting the public accessors of the class to make sure that > the class did *something* (anything) to produce the correct values. I don't care > how it did it, for example how the size and insets are being stored internally. > I wrote a test that checks that when I instantiate a class using its public > interface that its public accessors function correctly. This is textbook > black-box testing. > > In the CreateSolidBorder case, actually a fair amount of logic is going on in > the implementation. It takes a single int 3, calls gfx::Insets(int all) which > duplicates the int four times into the top, bottom, left and right fields. It > then stores those insets internally, and returns them if I call GetInsets(). > GetMinimumSize() calls width() which sums the left and right insets, producing > 6, and same for height(). My test doesn't care about any of those implementation > details; for example, the SolidSidedBorder could be modified to just store "int > inset_ = 3". Then GetInsets() would return gfx::Insets(inset_), resulting in > Insets(3, 3, 3, 3). And GetMinimumSize() could just return gfx::Size(inset_ * 2, > inset_ * 2). My test would still pass, just as long as the implementation > produces the correct results. And those are useful tests, because for example if > GetMinimumSize were to return just the left and top (forgetting to sum the > left+right, and top+bottom), it would return gfx::Size(3, 3) and fail the test. OK. I suppose I was thinking the implementation was too trivial for this test to be useful, but your explanation seems reasonable as well. > > Now that small nit aside, you still have not yet at all addressed the basic > question I asked on Monday which is: do you want me to abandon this CL, or can I > land it? If you are not happy with it in its current form, please give specific > actionable changes you would like me to make. Done. https://codereview.chromium.org/1517463003/diff/160001/ui/views/border_unitte... File ui/views/border_unittest.cc (right): https://codereview.chromium.org/1517463003/diff/160001/ui/views/border_unitte... ui/views/border_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. This should be updated I guess. https://codereview.chromium.org/1517463003/diff/160001/ui/views/border_unitte... ui/views/border_unittest.cc:73: MockPainter() : views::Painter() {} Is the : views::Painter() bit necessary? https://codereview.chromium.org/1517463003/diff/160001/ui/views/border_unitte... ui/views/border_unittest.cc:142: EXPECT_EQ(4u, draw_rect_calls.size()); ASSERT_EQ https://codereview.chromium.org/1517463003/diff/160001/ui/views/border_unitte... ui/views/border_unittest.cc:150: EXPECT_EQ(SK_ColorBLUE, draw_rect_calls[3].paint.getColor()); Can you change this so the ordering of the calls do not matter? https://codereview.chromium.org/1517463003/diff/160001/ui/views/border_unitte... ui/views/border_unittest.cc:166: EXPECT_EQ(1u, draw_rrect_calls.size()); ASSERT_EQ
Thanks. https://codereview.chromium.org/1517463003/diff/160001/ui/views/border_unitte... File ui/views/border_unittest.cc (right): https://codereview.chromium.org/1517463003/diff/160001/ui/views/border_unitte... ui/views/border_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/05/31 17:31:31, sadrul wrote: > This should be updated I guess. Done. https://codereview.chromium.org/1517463003/diff/160001/ui/views/border_unitte... ui/views/border_unittest.cc:73: MockPainter() : views::Painter() {} On 2016/05/31 17:31:30, sadrul wrote: > Is the : views::Painter() bit necessary? Done. https://codereview.chromium.org/1517463003/diff/160001/ui/views/border_unitte... ui/views/border_unittest.cc:142: EXPECT_EQ(4u, draw_rect_calls.size()); On 2016/05/31 17:31:30, sadrul wrote: > ASSERT_EQ Done. https://codereview.chromium.org/1517463003/diff/160001/ui/views/border_unitte... ui/views/border_unittest.cc:150: EXPECT_EQ(SK_ColorBLUE, draw_rect_calls[3].paint.getColor()); On 2016/05/31 17:31:31, sadrul wrote: > Can you change this so the ordering of the calls do not matter? Done. (Required operator< in the DrawRectCall and DrawRRectCall classes.) https://codereview.chromium.org/1517463003/diff/160001/ui/views/border_unitte... ui/views/border_unittest.cc:166: EXPECT_EQ(1u, draw_rrect_calls.size()); On 2016/05/31 17:31:30, sadrul wrote: > ASSERT_EQ Done.
https://codereview.chromium.org/1517463003/diff/240001/ui/views/border_unitte... File ui/views/border_unittest.cc (right): https://codereview.chromium.org/1517463003/diff/240001/ui/views/border_unitte... ui/views/border_unittest.cc:71: return std::vector<DrawRRectCall>(draw_rrect_calls_.begin(), Does this automatically sort the vector? https://codereview.chromium.org/1517463003/diff/240001/ui/views/border_unitte... ui/views/border_unittest.cc:166: EXPECT_EQ(SkRect::MakeLTRB(0, 0, 100, 3), draw_rect_calls[0].rect); Can't you just use set::find() instead?
(also, make sure to update the CL description, and the BUG list)
Description was changed from ========== Added comprehensive tests for views::Border. BorderTest.RoundedRectBorder is disabled due to http://crbug.com/565801. That will be fixed and enabled in a follow-up CL. BUG=568389, 134675 ========== to ========== Added comprehensive tests for views::Border. BUG=568389 ==========
> (also, make sure to update the CL description, and the BUG list) Done. https://codereview.chromium.org/1517463003/diff/240001/ui/views/border_unitte... File ui/views/border_unittest.cc (right): https://codereview.chromium.org/1517463003/diff/240001/ui/views/border_unitte... ui/views/border_unittest.cc:71: return std::vector<DrawRRectCall>(draw_rrect_calls_.begin(), On 2016/06/08 16:02:55, sadrul wrote: > Does this automatically sort the vector? Yes, because the std::set iterator intrinsically returns items in sorted order (that's why I used a set, and convert to a vector, as opposed to storing in a vector and then sorting). Added a note about this below. https://codereview.chromium.org/1517463003/diff/240001/ui/views/border_unitte... ui/views/border_unittest.cc:166: EXPECT_EQ(SkRect::MakeLTRB(0, 0, 100, 3), draw_rect_calls[0].rect); On 2016/06/08 16:02:55, sadrul wrote: > Can't you just use set::find() instead? I could but then I'd have to construct an entire SkPaint object instead of just checking the fields I care about (getColor()). I'd also have to upgrade the operator< to compare all the fields so equality would work correctly. Currently I just check the rect so I get a stable ordering; I don't need to check the paint as well.
lgtm https://codereview.chromium.org/1517463003/diff/240001/ui/views/border_unitte... File ui/views/border_unittest.cc (right): https://codereview.chromium.org/1517463003/diff/240001/ui/views/border_unitte... ui/views/border_unittest.cc:166: EXPECT_EQ(SkRect::MakeLTRB(0, 0, 100, 3), draw_rect_calls[0].rect); On 2016/06/09 01:49:38, Matt Giuca wrote: > On 2016/06/08 16:02:55, sadrul wrote: > > Can't you just use set::find() instead? > > I could but then I'd have to construct an entire SkPaint object instead of just > checking the fields I care about (getColor()). > > I'd also have to upgrade the operator< to compare all the fields so equality > would work correctly. Currently I just check the rect so I get a stable > ordering; I don't need to check the paint as well. OK. I would've preferred if you made DrawRectCall store a SkColor instead of the whole SkPaint, and used find() here for a DrawRectCall object instead of separately checking the rect and color. equality for DrawRectCall (i.e. operator==) makes much more sense than inequality (i.e. operator<)
https://codereview.chromium.org/1517463003/diff/240001/ui/views/border_unitte... File ui/views/border_unittest.cc (right): https://codereview.chromium.org/1517463003/diff/240001/ui/views/border_unitte... ui/views/border_unittest.cc:166: EXPECT_EQ(SkRect::MakeLTRB(0, 0, 100, 3), draw_rect_calls[0].rect); On 2016/06/13 00:47:38, sadrul wrote: > On 2016/06/09 01:49:38, Matt Giuca wrote: > > On 2016/06/08 16:02:55, sadrul wrote: > > > Can't you just use set::find() instead? > > > > I could but then I'd have to construct an entire SkPaint object instead of > just > > checking the fields I care about (getColor()). > > > > I'd also have to upgrade the operator< to compare all the fields so equality > > would work correctly. Currently I just check the rect so I get a stable > > ordering; I don't need to check the paint as well. > > OK. I would've preferred if you made DrawRectCall store a SkColor instead of the > whole SkPaint, and used find() here for a DrawRectCall object instead of > separately checking the rect and color. That means we have to specify exactly which attributes we care about in the DrawRectCall object, not in the tests (meaning the testing structure is fairly rigid). Everything that gets compared needs to be explicitly specified by the test (there's no way for one test to care about some attribute and another to not). I started doing this then realised we already have this situation: the RoundedRectBorder tests care about stroke width while the others don't (because the rects don't have stroke). So it's better to explicitly check for the properties you care about in the test. It also gives you better failure messages: "Value of: getStrokeWidth(); Actual: 4; Expected: 3", as opposed to "Value of: set.find(huge_object) != set.end(); Actual: false; Expected: true". There'd be no way to tell what failed. > equality for DrawRectCall (i.e. operator==) makes much more sense than > inequality (i.e. operator<) It has to be operator< for std::set to work. std::set decides whether objects are equal by calling operator< on them in both directions. It doesn't use operator==. I'm sorry to bring this up again but having to deal with, and argue, over these very specific details about how to best store and verify a list of calls (and get the most readable output messages)... this is exactly why we should be using GMock which handles all of this for you and does proper error messages.
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1517463003/260001
Message was sent while issue was closed.
Description was changed from ========== Added comprehensive tests for views::Border. BUG=568389 ========== to ========== Added comprehensive tests for views::Border. BUG=568389 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:260001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Added comprehensive tests for views::Border. BUG=568389 ========== to ========== Added comprehensive tests for views::Border. BUG=568389 Committed: https://crrev.com/92614dc154ad143a1ef9849ae0fe90c02df09f30 Cr-Commit-Position: refs/heads/master@{#399650} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/92614dc154ad143a1ef9849ae0fe90c02df09f30 Cr-Commit-Position: refs/heads/master@{#399650}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:260001) has been created in https://codereview.chromium.org/2060983004/ by zhenw@chromium.org. The reason for reverting is: This CL causes Win x64 Builder failure on chromium.perf. https://build.chromium.org/p/chromium.perf/builders/Win%20x64%20Builder/build... [5442/5460] LINK(DLL) chrome.dll chrome.dll.lib FAILED: chrome.dll chrome.dll.lib C:/b/depot_tools/python276_bin/python.exe gyp-win-tool link-wrapper environment.x64 False link.exe /nologo /IMPLIB:./chrome.dll.lib /DLL /OUT:./chrome.dll /PDB:./chrome.dll.pdb @./chrome.dll.rsp obj/chrome/browser/ui/ui.lib : fatal error LNK1107: invalid or corrupt file: cannot read at 0x14B19402 [5443/5460] LINK(DLL) chrome_child.dll chrome_child.dll.lib ninja: build stopped: subcommand failed. BUG=619949 .
Message was sent while issue was closed.
Description was changed from ========== Added comprehensive tests for views::Border. BUG=568389 Committed: https://crrev.com/92614dc154ad143a1ef9849ae0fe90c02df09f30 Cr-Commit-Position: refs/heads/master@{#399650} ========== to ========== Added comprehensive tests for views::Border. BUG=568389 Committed: https://crrev.com/92614dc154ad143a1ef9849ae0fe90c02df09f30 Cr-Commit-Position: refs/heads/master@{#399650} ==========
See bug comment #6. My CL clearly didn't break this builder. Re-landing.
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1517463003/260001
Message was sent while issue was closed.
Description was changed from ========== Added comprehensive tests for views::Border. BUG=568389 Committed: https://crrev.com/92614dc154ad143a1ef9849ae0fe90c02df09f30 Cr-Commit-Position: refs/heads/master@{#399650} ========== to ========== Added comprehensive tests for views::Border. BUG=568389 Committed: https://crrev.com/92614dc154ad143a1ef9849ae0fe90c02df09f30 Cr-Commit-Position: refs/heads/master@{#399650} ==========
Message was sent while issue was closed.
Committed patchset #13 (id:260001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Added comprehensive tests for views::Border. BUG=568389 Committed: https://crrev.com/92614dc154ad143a1ef9849ae0fe90c02df09f30 Cr-Commit-Position: refs/heads/master@{#399650} ========== to ========== Added comprehensive tests for views::Border. BUG=568389 Committed: https://crrev.com/92614dc154ad143a1ef9849ae0fe90c02df09f30 Committed: https://crrev.com/23a6dcc694a5d93d678c203dc834fb1b9dbd9fbf Cr-Original-Commit-Position: refs/heads/master@{#399650} Cr-Commit-Position: refs/heads/master@{#399852} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/23a6dcc694a5d93d678c203dc834fb1b9dbd9fbf Cr-Commit-Position: refs/heads/master@{#399852} |
