|
|
Created:
7 years, 2 months ago by mithro-old Modified:
7 years, 1 month ago Reviewers:
dstockwell, shans, Steve Block CC:
blink-reviews, shans, rjwright, alancutter (OOO until 2018), Mike Lawther (Google), dstockwell, Timothy Loh, darktears, Steve Block, dino_apple.com, Eric Willigers Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionTimingFunction test helper.
Makes it easier to use TimingFunction's in your tests as they can now be pretty
printed. (They already had an operator==.)
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161306
Patch Set 1 #Patch Set 2 : Rebasing onto master. #Patch Set 3 : Restore the RuntimeEnabledFeatures::setWebAnimationsEnabled flag. #Patch Set 4 : Moving test helper code into cpp file to fix linking issue. #
Total comments: 9
Patch Set 5 : Code review fixes. #
Total comments: 12
Patch Set 6 : CL fixes. #Patch Set 7 : Using more permission regex for windows. #
Created: 7 years, 1 month ago
Messages
Total messages: 12 (0 generated)
Hey guys, TimingFunctions are used quite a bit in our animation stuff, specially in the conversion code I've written. This makes them slightly easier to test with (first part of a split up patch). Tim
https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... File Source/core/platform/animation/TimingFunction.h (right): https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... Source/core/platform/animation/TimingFunction.h:319: friend void PrintTo(const ChainedTimingFunction&, ::std::ostream*, bool); I think it would be cleaner to put these methods on a CoreAnimationTimingFunctionTest object and just friend that here (or alternatively, if the TimingFunctionTestHelper is intended for use outside TimingFunctionTest, then on a TimingFunctionTestHelper object). That way we don't mix up the definition of TimingFunction with testing API, and we don't need to include testing-specific includes here (like <iosfwd>). https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... File Source/core/platform/animation/TimingFunctionTestHelperTest.cpp (right): https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelperTest.cpp:45: class TimingFunctionTestHelperTest : public ::testing::Test { I'm not a huge fan of these tests: (1) If TimingFunctionTestHelper is used elsewhere for testing then the expectations on those tests will break if TimingFunctionTestHelper does (2) It isn't necessary for TimingFunctionTestHelper to work in order for Blink to. (3) You're not really testing any invariants here - just that the arbitrary print string you selected is the one that gets printed. These tests make it harder to change that print string later.
The code in TimingFunctionTestHelper (the PrintTo) is shared by any test which uses a TimingFunction so they get a useful message rather then just "<18 byte object at 0xXXXXX>" in the test output. (They where extremely useful while testing the compositor conversion code and the TimingFunction reverse code which are in upcoming patches. Could also be used by the Timing test code in the future, etc). The reason I'm testing the string output of PrintTo functions is because its easy for them to end up wrong (IE The generic TimingFunction calls itself, or the defined PrintTo isn't actually matched, or the output doesn't match the input). Nobody wants to spend hours looking in wrong place when a PrintTo function ends up lieing to you. Just as proof these tests did pick up issues (IE All the issues that I mentioned above). Tim On 1 Nov 2013 07:22, <shans@chromium.org> wrote: > > https://codereview.chromium.**org/39223002/diff/200001/** > Source/core/platform/**animation/TimingFunction.h<https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/animation/TimingFunction.h> > File Source/core/platform/**animation/TimingFunction.h (right): > > https://codereview.chromium.**org/39223002/diff/200001/** > Source/core/platform/**animation/TimingFunction.h#**newcode319<https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/animation/TimingFunction.h#newcode319> > Source/core/platform/**animation/TimingFunction.h:**319: friend void > PrintTo(const ChainedTimingFunction&, ::std::ostream*, bool); > I think it would be cleaner to put these methods on a > CoreAnimationTimingFunctionTes**t object and just friend that here (or > alternatively, if the TimingFunctionTestHelper is intended for use > outside TimingFunctionTest, then on a TimingFunctionTestHelper object). > > That way we don't mix up the definition of TimingFunction with testing > API, and we don't need to include testing-specific includes here (like > <iosfwd>). > > https://codereview.chromium.**org/39223002/diff/200001/** > Source/core/platform/**animation/**TimingFunctionTestHelperTest.**cpp<https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/animation/TimingFunctionTestHelperTest.cpp> > File Source/core/platform/**animation/**TimingFunctionTestHelperTest.**cpp > (right): > > https://codereview.chromium.**org/39223002/diff/200001/** > Source/core/platform/**animation/**TimingFunctionTestHelperTest.** > cpp#newcode45<https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/animation/TimingFunctionTestHelperTest.cpp#newcode45> > Source/core/platform/**animation/**TimingFunctionTestHelperTest.**cpp:45: > class TimingFunctionTestHelperTest : public ::testing::Test { > I'm not a huge fan of these tests: > (1) If TimingFunctionTestHelper is used elsewhere for testing then the > expectations on those tests will break if TimingFunctionTestHelper does > (2) It isn't necessary for TimingFunctionTestHelper to work in order for > Blink to. > (3) You're not really testing any invariants here - just that the > arbitrary print string you selected is the one that gets printed. These > tests make it harder to change that print string later. > > https://codereview.chromium.**org/39223002/<https://codereview.chromium.org/3... > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... File Source/core/platform/animation/TimingFunctionTestHelper.cpp (right): https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelper.cpp:116: ASSERT(!recursiveGuard); I don't really like the idea of adding a param to this print method just so you can use it in this assertion, especially as the user of this method could pass in the wrong value. I don't think I've seen any other instance in Blink of params used solely for assertions. In any case, I think this needs to be ASSERT_UNUSED(), and you shouldn't name the param in the other definitions where you don't use it. https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... File Source/core/platform/animation/TimingFunctionTestHelperTest.cpp (right): https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelperTest.cpp:45: class TimingFunctionTestHelperTest : public ::testing::Test { I tend to agree. At a higher level, why do we have to print a timing function to a string in order to test it? Isn't it better to expose methods to interrogate the timing function directly, rather than using a print method and a regex to test via a string?
https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... File Source/core/platform/animation/TimingFunction.h (right): https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... Source/core/platform/animation/TimingFunction.h:319: friend void PrintTo(const ChainedTimingFunction&, ::std::ostream*, bool); On 2013/10/31 20:22:54, shans wrote: > I think it would be cleaner to put these methods on a > CoreAnimationTimingFunctionTest object and just friend that here (or > alternatively, if the TimingFunctionTestHelper is intended for use outside > TimingFunctionTest, then on a TimingFunctionTestHelper object). > > That way we don't mix up the definition of TimingFunction with testing API, and > we don't need to include testing-specific includes here (like <iosfwd>). The way PrintTo methods are defined is not controlled by myself. It is controlled via gtest. I could create a TimingFunctionTestHelper object which I can friend and then PrintTo methods call that? Will give that a try on Monday morning. https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... File Source/core/platform/animation/TimingFunctionTestHelper.cpp (right): https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelper.cpp:116: ASSERT(!recursiveGuard); On 2013/11/03 09:18:32, Steve Block wrote: > I don't really like the idea of adding a param to this print method just so you > can use it in this assertion, especially as the user of this method could pass > in the wrong value. I don't think I've seen any other instance in Blink of > params used solely for assertions. I can remove it if you feel strongly about it. Since the tests in TimingFunctionTestHelper will discover this issue pretty quickly, I'm not too concerned. > > In any case, I think this needs to be ASSERT_UNUSED(), and you shouldn't name > the param in the other definitions where you don't use it. Not sure what you mean by an ASSERT_UNUSED? I have use the same function signature on the other PrintTo methods otherwise when called below the compiler *will* select this function which is exactly what I'm trying to avoid against :). https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... File Source/core/platform/animation/TimingFunctionTestHelperTest.cpp (right): https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelperTest.cpp:45: class TimingFunctionTestHelperTest : public ::testing::Test { On 2013/11/03 09:18:32, Steve Block wrote: > I tend to agree. At a higher level, why do we have to print a timing function to > a string in order to test it? Isn't it better to expose methods to interrogate > the timing function directly, rather than using a print method and a regex to > test via a string? I think there is a misunderstand what is going on here. This code is testing the **PrintTo methods**, not the TimingFunction code. Nobody outside of the test for PrintTo itself should be calling PrintTo method directly. The PrintTo methods simple trivial at first and I originally didn't write any tests for them. However, it turns out there is actually some very complex behaviour going on behind the scenes in gtest. After having being bitten by this a number of times I ended up writing these tests to prove that the PrintTo code was working as expected (and actually discovered a couple of other minor issues in the process). On why the PrintTo methods are useful; the PrintTo functions are used during testing of other code via the testing framework to give you useful error messages when something goes wrong. If you did an EXPECT_EQ(CubicTimingFunction::preset(EaseIn), GenerateTimingFunction()) and the expectation fails, gtest will output the following. Without the PrintTo methods you get; EXPECT_EQ failed on Line XX in function XX: Expected not equal to actual Expected: CubicTimingFunction::preset(EaseIn) Which is: <Object 18 bytes> Actual: GenerateTimingFunction() Which is: <Object 18 bytes> With the PrintTo methods you get; EXPECT_EQ failed on Line XX in function XX: Expected not equal to actual Expected: CubicTimingFunction::preset(EaseIn) Which is: CubicTimingFunction(EaseIn, 0.0, 1.0, 0.0, 0.45) Actual: GenerateTimingFunction() Which is: LinearTimingFunction() Much easier to see why your test is failing! Does that make more sense?
I think if we can't get the <iosfwd> include and PrintTo method defines out of the core code, then this is not worth pursuing. If you can wrangle them out, then given that this has already been written I don't have any particular objection to checking it in. Don't spend much time on it though, getting the compositor integration working is *much* more important (and does not depend on this).
https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... File Source/core/platform/animation/TimingFunctionTestHelper.cpp (right): https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelper.cpp:116: ASSERT(!recursiveGuard); > Not sure what you mean by an ASSERT_UNUSED? ASSERT()s are only compiled in debug builds, and recursiveGuard is only used in this assert. So in a release build I think you'll get an 'unused param' warning on some platforms. ASSERT_UNUSED() works around this by 'using' the param even in release builds. https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... File Source/core/platform/animation/TimingFunctionTestHelperTest.cpp (right): https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelperTest.cpp:45: class TimingFunctionTestHelperTest : public ::testing::Test { > This code is testing the **PrintTo methods**, not the TimingFunction code. I see. I misunderstood your comment about the motivation for these printTo() methods. I thought you were suggesting they were to be used for testing, but in fact they're just to help debug failing tests.
On 2013/11/03 23:26:21, shans wrote: > I think if we can't get the <iosfwd> include and PrintTo method defines out of > the core code, then this is not worth pursuing. > > If you can wrangle them out, then given that this has already been written I > don't have any particular objection to checking it in. Don't spend much time on > it though, getting the compositor integration working is *much* more important > (and does not depend on this). Done! As you suggested friending a class made it much nicer. On 2013/11/03 23:29:44, Steve Block wrote: > https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... > File Source/core/platform/animation/TimingFunctionTestHelper.cpp (right): > > https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... > Source/core/platform/animation/TimingFunctionTestHelper.cpp:116: > ASSERT(!recursiveGuard); > > Not sure what you mean by an ASSERT_UNUSED? > ASSERT()s are only compiled in debug builds, and recursiveGuard is only used in > this assert. So in a release build I think you'll get an 'unused param' warning > on some platforms. ASSERT_UNUSED() works around this by 'using' the param even > in release builds. Remove. > https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... > File Source/core/platform/animation/TimingFunctionTestHelperTest.cpp (right): > > https://codereview.chromium.org/39223002/diff/200001/Source/core/platform/ani... > Source/core/platform/animation/TimingFunctionTestHelperTest.cpp:45: class > TimingFunctionTestHelperTest : public ::testing::Test { > > This code is testing the **PrintTo methods**, not the TimingFunction code. > I see. I misunderstood your comment about the motivation for these printTo() > methods. I thought you were suggesting they were to be used for testing, but in > fact they're just to help debug failing tests. Yeah, sorry about the confusion.
lgtm https://codereview.chromium.org/39223002/diff/330001/Source/core/platform/ani... File Source/core/platform/animation/TimingFunction.h (right): https://codereview.chromium.org/39223002/diff/330001/Source/core/platform/ani... Source/core/platform/animation/TimingFunction.h:96: // Forward declare so we can friend it below. Don't used in production code! s/used/use https://codereview.chromium.org/39223002/diff/330001/Source/core/platform/ani... File Source/core/platform/animation/TimingFunctionTestHelper.cpp (right): https://codereview.chromium.org/39223002/diff/330001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelper.cpp:32: Superfluous blank line https://codereview.chromium.org/39223002/diff/330001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelper.cpp:39: void PrintTo(const LinearTimingFunction& timefunction, ::std::ostream* os) s/timeFunction/timingFunction https://codereview.chromium.org/39223002/diff/330001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelper.cpp:44: void PrintTo(const CubicBezierTimingFunction& timefunction, ::std::ostream* os) Function and method names start with a lower case letter - printTo() https://codereview.chromium.org/39223002/diff/330001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelper.cpp:94: class ChainedTimingFunctionPrintTo { Is ChainedTimingFunctionPrinter a better name? https://codereview.chromium.org/39223002/diff/330001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelper.cpp:116: void PrintTo(const ChainedTimingFunction& timefunction, ::std::ostream* os) Do we need this wrapper? Can the generic PrintTo() call ChainedTimingFunctionPrintTo::PrintTo directly?
https://codereview.chromium.org/39223002/diff/330001/Source/core/platform/ani... File Source/core/platform/animation/TimingFunction.h (right): https://codereview.chromium.org/39223002/diff/330001/Source/core/platform/ani... Source/core/platform/animation/TimingFunction.h:96: // Forward declare so we can friend it below. Don't used in production code! On 2013/11/04 23:05:02, Steve Block wrote: > s/used/use Done. https://codereview.chromium.org/39223002/diff/330001/Source/core/platform/ani... File Source/core/platform/animation/TimingFunctionTestHelper.cpp (right): https://codereview.chromium.org/39223002/diff/330001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelper.cpp:32: On 2013/11/04 23:05:02, Steve Block wrote: > Superfluous blank line Done. https://codereview.chromium.org/39223002/diff/330001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelper.cpp:39: void PrintTo(const LinearTimingFunction& timefunction, ::std::ostream* os) On 2013/11/04 23:05:02, Steve Block wrote: > s/timeFunction/timingFunction Done. https://codereview.chromium.org/39223002/diff/330001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelper.cpp:44: void PrintTo(const CubicBezierTimingFunction& timefunction, ::std::ostream* os) On 2013/11/04 23:05:02, Steve Block wrote: > Function and method names start with a lower case letter - printTo() Don't get to control the name of this method. Set by gtest. https://codereview.chromium.org/39223002/diff/330001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelper.cpp:94: class ChainedTimingFunctionPrintTo { On 2013/11/04 23:05:02, Steve Block wrote: > Is ChainedTimingFunctionPrinter a better name? Done. https://codereview.chromium.org/39223002/diff/330001/Source/core/platform/ani... Source/core/platform/animation/TimingFunctionTestHelper.cpp:116: void PrintTo(const ChainedTimingFunction& timefunction, ::std::ostream* os) On 2013/11/04 23:05:02, Steve Block wrote: > Do we need this wrapper? Can the generic PrintTo() call > ChainedTimingFunctionPrintTo::PrintTo directly? Yes, otherwise when you try and print a ChainedTimingFunction it will get the generic method (*not* the base one). See https://sites.google.com/a/chromium.org/dev/blink/unittesting#TOC-Pretty-Prin...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mithro@mithis.com/39223002/490001
Message was sent while issue was closed.
Change committed as 161306 |