skia: Add 'skia_unittests' test suite for this directory.

skia: Add 'skia_unittests' test suite for this directory.

This should be a lightweight test suite that should allow a dev changing
something in skia/ext to test it much more quickly than having to build
all 'unit_tests' target just to run the tests from skia/ext.

BUG=444058
TEST=skia_unittests links and runs fine.
R=danakj@chromium.org,thakis@chromium.org

Committed: https://crrev.com/c87c3d2d949f9ba4f17627dc023b303c75b7635f
Cr-Commit-Position: refs/heads/master@{#310310}

[tfarina, 2014-12-19 00:28:19]

Hey Dana, PTAL!

Nico, could you rubber stamp this? Sorry, I had discussed doing this with Dana,
Scott and Reed on e-mail.

[danakj, 2014-12-19 01:36:38]

Should remove from unit_tests too, right?

[tfarina, 2014-12-19 02:00:31]

On 2014/12/19 01:36:38, danakj wrote:
> Should remove from unit_tests too, right?

Not now, I guess. Otherwise, won't we loose the coverage?

First we need to get skia_unittests running on the bots, to then be able to
remove from unit_tests.

[danakj, 2014-12-19 16:27:16]

On 2014/12/19 02:00:31, tfarina wrote:
> On 2014/12/19 01:36:38, danakj wrote:
> > Should remove from unit_tests too, right?
> 
> Not now, I guess. Otherwise, won't we loose the coverage?
> 
> First we need to get skia_unittests running on the bots, to then be able to
> remove from unit_tests.

How about throwing a TODO in the unit_tests gyp where the skia stuff is then?

[tfarina, 2014-12-19 16:45:18]

On 2014/12/19 16:27:16, danakj wrote:
> On 2014/12/19 02:00:31, tfarina wrote:
> > On 2014/12/19 01:36:38, danakj wrote:
> > > Should remove from unit_tests too, right?
> > 
> > Not now, I guess. Otherwise, won't we loose the coverage?
> > 
> > First we need to get skia_unittests running on the bots, to then be able to
> > remove from unit_tests.
> 
> How about throwing a TODO in the unit_tests gyp where the skia stuff is then?

Sure. This sounds good. I will add it later tonight (with a bug filed, which I'm
doing right now).

[tfarina, 2014-12-20 00:49:05]

Dana, could you please take another look?

It is up and running on the bots! Thanks to the recipes magic!

http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

Cheers,

[danakj, 2014-12-23 17:40:52]

https://codereview.chromium.org/815023003/diff/120001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/815023003/diff/120001/build/all.gyp#newcode866
build/all.gyp:866: '../chrome/chrome.gyp:unit_tests_apk',
is there no skia_unittests_apk?


http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
i don't see this test running on the android bot as a step, should it be?

https://codereview.chromium.org/815023003/diff/120001/build/all.gyp#newcode1026
build/all.gyp:1026: '../chrome/chrome.gyp:unit_tests',
should it be added here?

https://codereview.chromium.org/815023003/diff/120001/build/all.gyp#newcode1161
build/all.gyp:1161: '../chrome/chrome.gyp:unit_tests',
should it be added here?

https://codereview.chromium.org/815023003/diff/120001/build/all.gyp#newcode1294
build/all.gyp:1294: '../ui/snapshot/snapshot.gyp:snapshot_unittests',
should it be added here?

[tfarina, 2014-12-24 21:23:06]

tfarina@chromium.org changed reviewers:
+ thestig@chromium.org

[tfarina, 2014-12-24 21:23:08]

Lei, could you review the memory changes?

https://codereview.chromium.org/815023003/diff/120001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/815023003/diff/120001/build/all.gyp#newcode866
build/all.gyp:866: '../chrome/chrome.gyp:unit_tests_apk',
On 2014/12/23 17:40:52, danakj wrote:
> is there no skia_unittests_apk?
> 
> 
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> i don't see this test running on the android bot as a step, should it be?

Done.

https://codereview.chromium.org/815023003/diff/120001/build/all.gyp#newcode1026
build/all.gyp:1026: '../chrome/chrome.gyp:unit_tests',
On 2014/12/23 17:40:52, danakj wrote:
> should it be added here?

Done.

https://codereview.chromium.org/815023003/diff/120001/build/all.gyp#newcode1161
build/all.gyp:1161: '../chrome/chrome.gyp:unit_tests',
On 2014/12/23 17:40:52, danakj wrote:
> should it be added here?

Done.

https://codereview.chromium.org/815023003/diff/120001/build/all.gyp#newcode1294
build/all.gyp:1294: '../ui/snapshot/snapshot.gyp:snapshot_unittests',
On 2014/12/23 17:40:52, danakj wrote:
> should it be added here?

Done.

https://codereview.chromium.org/815023003/diff/120001/build/all.gyp#newcode1294
build/all.gyp:1294: '../ui/snapshot/snapshot.gyp:snapshot_unittests',
On 2014/12/23 17:40:52, danakj wrote:
> should it be added here?

Done.

[Alexander Potapenko, 2014-12-25 08:27:58]

glider@chromium.org changed reviewers:
+ glider@chromium.org

[Alexander Potapenko, 2014-12-25 08:27:59]

Memory changes LGTM

[danakj, 2014-12-30 16:05:27]

How come skia_unittests isn't running here?
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
 But unit_tests is.

[tfarina, 2014-12-30 16:12:40]

I don't know. I didn't see anything related to android in testing/buildbot. So
it might require changes in the 'buildbot' repo.

Let's ask some Android folks.

CCing Sami, Chris.

[Sami, 2014-12-30 17:03:04]

On 2014/12/30 16:12:40, tfarina wrote:
> I don't know. I didn't see anything related to android in testing/buildbot. So
> it might require changes in the 'buildbot' repo.
> 
> Let's ask some Android folks.
> 
> CCing Sami, Chris.

I believe the list you're looking for is here:
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...

[tfarina, 2014-12-30 23:46:54]

On 2014/12/30 17:03:04, Sami wrote:
> On 2014/12/30 16:12:40, tfarina wrote:
> > I don't know. I didn't see anything related to android in testing/buildbot.
So
> > it might require changes in the 'buildbot' repo.
> > 
> > Let's ask some Android folks.
> > 
> > CCing Sami, Chris.
> 
> I believe the list you're looking for is here:
>
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...

[tfarina, 2014-12-30 23:47:24]

On 2014/12/30 17:03:04, Sami wrote:
> On 2014/12/30 16:12:40, tfarina wrote:
> > I don't know. I didn't see anything related to android in testing/buildbot.
So
> > it might require changes in the 'buildbot' repo.
> > 
> > Let's ask some Android folks.
> > 
> > CCing Sami, Chris.
> 
> I believe the list you're looking for is here:
>
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...

Cool! Thanks a lot for pointing this out Sami! I have uploaded CL
https://codereview.chromium.org/792043006 for review.

[tfarina, 2015-01-05 19:05:43]

Dana, ping? Does this lgty now?

[danakj, 2015-01-05 19:28:15]

LGTM you'll need some more owners tho.

[tfarina, 2015-01-05 19:51:27]

tfarina@chromium.org changed reviewers:
+ sky@chromium.org
- thestig@chromium.org

[tfarina, 2015-01-05 19:52:37]

Nico, could you approve build/ and skia/ changes?

Scott, could you approve chrome/ and testing/buildbot/ changes?

[Nico, 2015-01-05 20:33:52]

What's the motivation for doing this? This'll require lots of bots surgery for
very little gain as far as I can tell as there isn't much code in skia/ (and I
suppose over time it should become less)

[tfarina, 2015-01-05 20:56:48]

On 2015/01/05 20:33:52, Nico wrote:
> What's the motivation for doing this?
I have a slow machine, I don't have a z620 or access to goma. I can't have the
luxury of having to wait hours for unit_tests to link.

> This'll require lots of bots surgery
The bots changes are all here. Except from one for the Android side, that I
already did. https://codereview.chromium.org/792043006

> for
> very little gain as far as I can tell as there isn't much code in skia/ (and I
> suppose over time it should become less)

[Nico, 2015-01-05 22:24:20]

I'm almost certain that there are bots that don't use these json files. (For
example,
https://code.google.com/p/chromium/codesearch#chromium/build/masters/master.c...
looks like a master-side-configured test list that needs a master restart and
stuff.)

`git log  --author=tfarina skia/ext` suggests that you touched skia/ext 3 times
in all of 2014. Do we really need to add a test binary for this?

https://codereview.chromium.org/815023003/diff/180001/testing/buildbot/chromi...
File testing/buildbot/chromium.chromiumos.json (right):

https://codereview.chromium.org/815023003/diff/180001/testing/buildbot/chromi...
testing/buildbot/chromium.chromiumos.json:167: "skia_unittests"
This list is almost alphabetized, consider moving this up a bit

https://codereview.chromium.org/815023003/diff/180001/testing/buildbot/chromi...
File testing/buildbot/chromium.fyi.json (right):

https://codereview.chromium.org/815023003/diff/180001/testing/buildbot/chromi...
testing/buildbot/chromium.fyi.json:52: "skia_unittests"
likewise

https://codereview.chromium.org/815023003/diff/180001/testing/buildbot/chromi...
testing/buildbot/chromium.fyi.json:119: "skia_unittests"
likewise (also for the rest of this file)

[tfarina, 2015-01-06 00:54:43]

https://codereview.chromium.org/815023003/diff/180001/testing/buildbot/chromi...
File testing/buildbot/chromium.chromiumos.json (right):

https://codereview.chromium.org/815023003/diff/180001/testing/buildbot/chromi...
testing/buildbot/chromium.chromiumos.json:167: "skia_unittests"
On 2015/01/05 22:24:20, Nico wrote:
> This list is almost alphabetized, consider moving this up a bit

Done.

https://codereview.chromium.org/815023003/diff/180001/testing/buildbot/chromi...
File testing/buildbot/chromium.fyi.json (right):

https://codereview.chromium.org/815023003/diff/180001/testing/buildbot/chromi...
testing/buildbot/chromium.fyi.json:52: "skia_unittests"
On 2015/01/05 22:24:20, Nico wrote:
> likewise

Done.

https://codereview.chromium.org/815023003/diff/180001/testing/buildbot/chromi...
testing/buildbot/chromium.fyi.json:119: "skia_unittests"
On 2015/01/05 22:24:20, Nico wrote:
> likewise (also for the rest of this file)

Done.

[tfarina, 2015-01-06 19:05:07]

Nico, does this lgty now?

[Nico, 2015-01-06 19:27:50]

You didn't reply to

"""I'm almost certain that there are bots that don't use these json files.
(For
example,
https://code.google.com/p/chromium/codesearch#chromium/
build/masters/master.chromium.memory.fyi/master.cfg&l=216
looks like a master-side-configured test list that needs a master restart
and
stuff.)

`git log  --author=tfarina skia/ext` suggests that you touched skia/ext 3
times
in all of 2014. Do we really need to add a test binary for this?"""

On Tue, Jan 6, 2015 at 11:05 AM, <tfarina@chromium.org> wrote:

> Nico, does this lgty now?
>
> https://codereview.chromium.org/815023003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tfarina, 2015-01-06 20:13:14]

On Tue, Jan 6, 2015 at 5:27 PM, Nico Weber <thakis@chromium.org> wrote:

> You didn't reply to
>
> Having it in unit_tests certainly does not encourage me to touch it more.
For me, personally, having skia_unittests target has 3 benefits; a
lightweight target that I can build and run in few seconds, do not have to
build unit_tests and wait it for link (which takes more than 1 hour for
sure), and later few tests in unit_tests is a good thing in my opinion
(as extensions_unittests
which removed almost 10% from unit_tests, 600 test cases). If thinkg making
unit_tests smaller has a huge benefit.

Also, it seems the practice of top-level directories to have their unit
tests target.

--
Thiago

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-01-06 20:17:27]

On Tue, Jan 6, 2015 at 12:13 PM, Thiago Farina <tfarina@chromium.org> wrote:

>
>
> On Tue, Jan 6, 2015 at 5:27 PM, Nico Weber <thakis@chromium.org> wrote:
>
>> You didn't reply to
>>
>> Having it in unit_tests certainly does not encourage me to touch it more.
> For me, personally, having skia_unittests target has 3 benefits; a
> lightweight target that I can build and run in few seconds, do not have to
> build unit_tests and wait it for link (which takes more than 1 hour for
> sure), and later few tests in unit_tests is a good thing in my opinion (as
extensions_unittests
> which removed almost 10% from unit_tests, 600 test cases). If thinkg making
> unit_tests smaller has a huge benefit.
>

I encouraged this patch because I think splitting up the monolithic
unit_tests target into smaller more focused binaries would be beneficial
for developers and bot cycle times (smaller thing to build and run if you
change code in skia/ in this case).


>
> Also, it seems the practice of top-level directories to have their unit
> tests target.
>
> --
> Thiago
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2015-01-06 20:32:23]

I'm not arguing that this doesn't have a benefit, I'm just not sure if it's
worth the churn. But if you're going to deal with the fallout, lgtm, I suppose.

https://codereview.chromium.org/815023003/diff/200001/tools/valgrind/chrome_t...
File tools/valgrind/chrome_tests.py (right):

https://codereview.chromium.org/815023003/diff/200001/tools/valgrind/chrome_t...
tools/valgrind/chrome_tests.py:712: "skia_unittests": TestSkia,
for some reason, everything else has two entries here, with and without
"_unittests' suffix. I'd add a "skia": TestSkia, entry in front of this line too

[tfarina, 2015-01-06 23:25:39]

https://codereview.chromium.org/815023003/diff/200001/tools/valgrind/chrome_t...
File tools/valgrind/chrome_tests.py (right):

https://codereview.chromium.org/815023003/diff/200001/tools/valgrind/chrome_t...
tools/valgrind/chrome_tests.py:712: "skia_unittests": TestSkia,
On 2015/01/06 20:32:23, Nico wrote:
> for some reason, everything else has two entries here, with and without
> "_unittests' suffix. I'd add a "skia": TestSkia, entry in front of this line
too

Done.

[tfarina, 2015-01-06 23:26:01]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2015-01-06 23:27:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/815023003/220001

[commit-bot: I haz the power, 2015-01-06 23:35:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-06 23:35:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tfarina, 2015-01-06 23:38:46]

On 2015/01/06 23:35:29, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Scott, could you approve this?

[sky, 2015-01-07 16:04:35]

LGTM

[tfarina, 2015-01-07 16:12:39]

On 2015/01/07 16:04:35, sky wrote:
> LGTM

Thanks Scott.

Pushing to CQ!

[tfarina, 2015-01-07 16:13:16]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2015-01-07 16:13:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/815023003/220001

[commit-bot: I haz the power, 2015-01-07 17:23:21]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-01-07 17:24:32]

Patchset 12 (id:??) landed as
https://crrev.com/c87c3d2d949f9ba4f17627dc023b303c75b7635f
Cr-Commit-Position: refs/heads/master@{#310310}

[Marijn Kruisselbrink, 2015-01-07 17:50:27]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/816803003/ by mek@chromium.org.

The reason for reverting is: Fails linking on the Mac GN builder:
http://build.chromium.org/p/chromium.mac/builders/Mac%20GN/builds/2094
duplicate symbol __ZN3gfx27SkMatrixToCGAffineTransformERK8SkMatrix in:
    obj/skia/ext/skia_unittests.skia_utils_mac_unittest.o
    obj/skia/ext/skia.skia_utils_mac.o
(and more similar issues).

[tfarina, 2015-01-07 18:07:55]

On Wed, Jan 7, 2015 at 3:50 PM, <mek@chromium.org> wrote:

> A revert of this CL (patchset #12 id:220001) has been created in
> https://codereview.chromium.org/816803003/ by mek@chromium.org.
>
> The reason for reverting is: Fails linking on the Mac GN builder:
> http://build.chromium.org/p/chromium.mac/builders/Mac%20GN/builds/2094
> duplicate symbol __ZN3gfx27SkMatrixToCGAffineTransformERK8SkMatrix in:
>     obj/skia/ext/skia_unittests.skia_utils_mac_unittest.o
>     obj/skia/ext/skia.skia_utils_mac.o
> (and more similar issues).
>
> Thanks! Fix is coming. Surprisingly
https://chromium.googlesource.com/chromium/src/+/master/skia/ext/skia_utils_m...
is including skia_utils_mac.mm rather than skia_utils_mac.h, causing the
duplicate definition of symbols looks like.


> https://codereview.chromium.org/815023003/
>



-- 
Thiago Farina

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2015-01-15 18:41:06]

Short check after this has been in the tree for 1 week: skia_unittests still
doesn't run on most bots (e.g.
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Tests...),
and it caused a bunch of trouble already. I continue to think that the benefits
from this change aren't worth the cost that this change has.

[sky, 2015-01-15 19:23:55]

+1

On Thu, Jan 15, 2015 at 10:41 AM,  <thakis@chromium.org> wrote:
> Short check after this has been in the tree for 1 week: skia_unittests still
> doesn't run on most bots (e.g.
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Tests...),
> and it caused a bunch of trouble already. I continue to think that the
> benefits
> from this change aren't worth the cost that this change has.
>
> https://codereview.chromium.org/815023003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tfarina, 2015-01-15 21:22:53]

On 2015/01/15 18:41:06, Nico wrote:
> Short check after this has been in the tree for 1 week: skia_unittests still
> doesn't run on most bots (e.g.
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Tests...),
> and it caused a bunch of trouble already. I continue to think that the
benefits
> from this change aren't worth the cost that this change has.

I'm on it, I will get to finish it. The url you linked above has skia_unittests
running on it btw!