[Testing] Run libaddressinput_unittests on bots

[Testing] Run libaddressinput_unittests on bots

BUG=585151
TEST=bots

Committed: https://crrev.com/6ddda6d1f93fddbe7656dd237344f5f571df8da2
Cr-Commit-Position: refs/heads/master@{#374933}

[Mathieu, 2016-02-08 21:44:17]

Patchset #1 (id:1) has been deleted

[M-A Ruel, 2016-02-08 21:48:59]

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

[M-A Ruel, 2016-02-08 21:49:00]

https://codereview.chromium.org/1681663002/diff/20001/third_party/libaddressi...
File third_party/libaddressinput/libaddressinput_unittests.isolate (right):

https://codereview.chromium.org/1681663002/diff/20001/third_party/libaddressi...
third_party/libaddressinput/libaddressinput_unittests.isolate:6: ['OS=="linux"
or OS=="mac" or OS=="win" or OS=="android"', {
Remove android

https://codereview.chromium.org/1681663002/diff/20001/third_party/libphonenum...
File third_party/libphonenumber/libphonenumber.gyp (right):

https://codereview.chromium.org/1681663002/diff/20001/third_party/libphonenum...
third_party/libphonenumber/libphonenumber.gyp:160: 'targets': [
You are already inside a targets section. You must get out of the target first.

[Mathieu, 2016-02-09 00:53:29]

Description was changed from

==========
[Testing] Run libaddressinput_unittests and libphonenumber_unittests on bots

BUG=585151
TEST=bots
==========

to

==========
[Testing] Run libaddressinput_unittests on bots

BUG=585151
TEST=bots
==========

[Mathieu, 2016-02-09 00:53:29]

mathp@chromium.org changed reviewers:
+ thakis@chromium.org

[Mathieu, 2016-02-09 00:55:50]

Hi Nico, 

please have a look

https://codereview.chromium.org/1681663002/diff/20001/third_party/libphonenum...
File third_party/libphonenumber/libphonenumber.gyp (right):

https://codereview.chromium.org/1681663002/diff/20001/third_party/libphonenum...
third_party/libphonenumber/libphonenumber.gyp:160: 'targets': [
On 2016/02/08 21:49:00, M-A Ruel wrote:
> You are already inside a targets section. You must get out of the target
first.

Done.

[Mathieu, 2016-02-09 00:57:19]

https://codereview.chromium.org/1681663002/diff/20001/third_party/libaddressi...
File third_party/libaddressinput/libaddressinput_unittests.isolate (right):

https://codereview.chromium.org/1681663002/diff/20001/third_party/libaddressi...
third_party/libaddressinput/libaddressinput_unittests.isolate:6: ['OS=="linux"
or OS=="mac" or OS=="win" or OS=="android"', {
On 2016/02/08 21:48:59, M-A Ruel wrote:
> Remove android

Done.

[Nico, 2016-02-09 01:26:10]

thanks!

https://codereview.chromium.org/1681663002/diff/60001/testing/buildbot/chromi...
File testing/buildbot/chromium.linux.json (right):

https://codereview.chromium.org/1681663002/diff/60001/testing/buildbot/chromi...
testing/buildbot/chromium.linux.json:757: {
can you add it to the release bot too? else it won't run on try jobs (which do
release bots with dcheck_always_on)

[Mathieu, 2016-02-09 01:32:05]

https://codereview.chromium.org/1681663002/diff/60001/testing/buildbot/chromi...
File testing/buildbot/chromium.linux.json (right):

https://codereview.chromium.org/1681663002/diff/60001/testing/buildbot/chromi...
testing/buildbot/chromium.linux.json:757: {
On 2016/02/09 01:26:10, Nico wrote:
> can you add it to the release bot too? else it won't run on try jobs (which do
> release bots with dcheck_always_on)

Done. Let me know if this is what you meant!

[Nico, 2016-02-09 01:33:01]

yes, that's what I meant. Can you do it on mac and win too? (else it's possible
some cl makes it through the c

[Nico, 2016-02-09 01:33:13]

q but then breaks the mac/win main waterfall dbg bots)

[Mathieu, 2016-02-09 01:37:38]

Done, PTAL!

[Nico, 2016-02-09 01:38:32]

lgtm, thanks :-)

[M-A Ruel, 2016-02-09 03:00:35]

Error opening "src/third_party/libaddressinput/src/testdata/countryinfo.txt".

Looks like you forgot to map the test data.

[Nico, 2016-02-09 03:02:39]

On 2016/02/09 03:00:35, M-A Ruel wrote:
> Error opening "src/third_party/libaddressinput/src/testdata/countryinfo.txt".
> 
> Looks like you forgot to map the test data.

(examples for how to do this:
https://code.google.com/p/chromium/codesearch#search/&q=test.?data%20file:%5C...)

[Mathieu, 2016-02-09 15:09:45]

On 2016/02/09 03:02:39, Nico wrote:
> On 2016/02/09 03:00:35, M-A Ruel wrote:
> > Error opening
"src/third_party/libaddressinput/src/testdata/countryinfo.txt".
> > 
> > Looks like you forgot to map the test data.
> 
> (examples for how to do this:
>
https://code.google.com/p/chromium/codesearch#search/&q=test.?data%20file:%5C...)

Hi, I'm hitting a snag.

The tests expect to read from TEST_DATA_DIR, which is set in
libaddressinput.gyp. In patchset 6 and 7, I have two different attempts to fix
it, with none working. 

Patchset 7 feels like it should work, but the tests end up looking for
"$!PRODUCT_DIR/libaddressinput_test_data//countryinfo.txt". Anyway to have a gyp
define that will reference the proper dir?

Thanks

[Mathieu, 2016-02-09 15:13:48]

On 2016/02/09 15:09:45, Mathieu Perreault wrote:
> On 2016/02/09 03:02:39, Nico wrote:
> > On 2016/02/09 03:00:35, M-A Ruel wrote:
> > > Error opening
> "src/third_party/libaddressinput/src/testdata/countryinfo.txt".
> > > 
> > > Looks like you forgot to map the test data.
> > 
> > (examples for how to do this:
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=test.?data%20file:%5C...)
> 
> Hi, I'm hitting a snag.
> 
> The tests expect to read from TEST_DATA_DIR, which is set in
> libaddressinput.gyp. In patchset 6 and 7, I have two different attempts to fix
> it, with none working. 
> 
> Patchset 7 feels like it should work, but the tests end up looking for
> "$!PRODUCT_DIR/libaddressinput_test_data//countryinfo.txt". Anyway to have a
gyp
> define that will reference the proper dir?
> 
> Thanks

To be clear, Patchset 6 allows me to run the tests from <checkout>/src, but not
in swarming.
Patchset 7 has the error above where tests can't be run locally or in swarming.

[M-A Ruel, 2016-02-09 15:38:28]

https://codereview.chromium.org/1681663002/diff/140001/third_party/libaddress...
File third_party/libaddressinput/libaddressinput_unittests.isolate (right):

https://codereview.chromium.org/1681663002/diff/140001/third_party/libaddress...
third_party/libaddressinput/libaddressinput_unittests.isolate:13:
'src/testdata/',
The path here is relative to the directory containing this file so it looks like
it's fine. The problem may be directory walking done by the test.

You didn't trigger try jobs so I can't look at the isolated file or swarming
task result.

[Mathieu, 2016-02-09 16:10:11]

https://codereview.chromium.org/1681663002/diff/140001/third_party/libaddress...
File third_party/libaddressinput/libaddressinput_unittests.isolate (right):

https://codereview.chromium.org/1681663002/diff/140001/third_party/libaddress...
third_party/libaddressinput/libaddressinput_unittests.isolate:13:
'src/testdata/',
On 2016/02/09 15:38:28, M-A Ruel wrote:
> The path here is relative to the directory containing this file so it looks
like
> it's fine. The problem may be directory walking done by the test.
> 
> You didn't trigger try jobs so I can't look at the isolated file or swarming
> task result.

Triggered try jobs, with interesting pre-patch failures?

[Mathieu, 2016-02-09 16:14:42]

On 2016/02/09 16:10:11, Mathieu Perreault wrote:
>
https://codereview.chromium.org/1681663002/diff/140001/third_party/libaddress...
> File third_party/libaddressinput/libaddressinput_unittests.isolate (right):
> 
>
https://codereview.chromium.org/1681663002/diff/140001/third_party/libaddress...
> third_party/libaddressinput/libaddressinput_unittests.isolate:13:
> 'src/testdata/',
> On 2016/02/09 15:38:28, M-A Ruel wrote:
> > The path here is relative to the directory containing this file so it looks
> like
> > it's fine. The problem may be directory walking done by the test.
> > 
> > You didn't trigger try jobs so I can't look at the isolated file or swarming
> > task result.
> 
> Triggered try jobs, with interesting pre-patch failures?

The test literally reads from the TEST_DATA_DIR set in gyp (see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libadd...).

How can we set it to a swarming friendly location?

[Nico, 2016-02-09 16:21:10]

On 2016/02/09 16:14:42, Mathieu Perreault wrote:
> On 2016/02/09 16:10:11, Mathieu Perreault wrote:
> >
>
https://codereview.chromium.org/1681663002/diff/140001/third_party/libaddress...
> > File third_party/libaddressinput/libaddressinput_unittests.isolate (right):
> > 
> >
>
https://codereview.chromium.org/1681663002/diff/140001/third_party/libaddress...
> > third_party/libaddressinput/libaddressinput_unittests.isolate:13:
> > 'src/testdata/',
> > On 2016/02/09 15:38:28, M-A Ruel wrote:
> > > The path here is relative to the directory containing this file so it
looks
> > like
> > > it's fine. The problem may be directory walking done by the test.
> > > 
> > > You didn't trigger try jobs so I can't look at the isolated file or
swarming
> > > task result.
> > 
> > Triggered try jobs, with interesting pre-patch failures?
> 
> The test literally reads from the TEST_DATA_DIR set in gyp (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libadd...).
> 
> How can we set it to a swarming friendly location?

Oh, that won't work. Many other tests read test data
(https://code.google.com/p/chromium/codesearch#search/&q=test.?data%20file:%5C...),
can you check what they do?

[M-A Ruel, 2016-02-09 18:48:58]

https://chromium-swarm.appspot.com/user/task/2cdfb974c3049910

Looks like you need to map in addition:

<(PRODUCT_DIR)/libaddressinput_test_data/

[Mathieu, 2016-02-09 21:18:45]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-02-09 21:18:45]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1681663002/#ps150007
(title: "split target")

[commit-bot: I haz the power, 2016-02-09 21:20:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1681663002/150007
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1681663002/150007

[commit-bot: I haz the power, 2016-02-09 22:38:07]

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

[commit-bot: I haz the power, 2016-02-09 22:38:07]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2016-02-10 03:34:38]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-02-10 03:34:39]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1681663002/#ps190001
(title: "name fi")

[commit-bot: I haz the power, 2016-02-10 03:35:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1681663002/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1681663002/190001

[commit-bot: I haz the power, 2016-02-10 04:17:00]

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

[commit-bot: I haz the power, 2016-02-10 04:17:01]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2016-02-10 13:32:25]

Patchset #8 (id:150007) has been deleted

[Mathieu, 2016-02-10 13:32:36]

Patchset #8 (id:170001) has been deleted

[Mathieu, 2016-02-10 13:32:43]

Patchset #7 (id:140001) has been deleted

[Mathieu, 2016-02-10 13:32:53]

Patchset #5 (id:100001) has been deleted

[Mathieu, 2016-02-10 13:35:34]

On 2016/02/10 04:17:01, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

M-A, would you have an idea why it only fails on one bot? I believe it's looks
for the data in the right place, it just seems like src/testdata is not mapped
out on the swarming server for that bot.

[Nico, 2016-02-10 13:44:46]

That's a gn bot, right? Since you changed a .gyp file, do you need to change a
.gn file too?

[Mathieu, 2016-02-10 14:55:21]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-02-10 14:55:21]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1681663002/#ps210001
(title: "gn")

[commit-bot: I haz the power, 2016-02-10 14:56:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1681663002/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1681663002/210001

[commit-bot: I haz the power, 2016-02-10 14:59:20]

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

[commit-bot: I haz the power, 2016-02-10 14:59:21]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[M-A Ruel, 2016-02-10 15:17:27]

maruel@chromium.org changed reviewers:
+ dpranke@chromium.org

[M-A Ruel, 2016-02-10 15:17:28]

+Dirk for the mechanism of adding the GN target properly.

[Mathieu, 2016-02-10 15:17:37]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-02-10 15:17:38]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1681663002/#ps230001
(title: "rebase")

[commit-bot: I haz the power, 2016-02-10 15:17:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1681663002/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1681663002/230001

[commit-bot: I haz the power, 2016-02-10 15:46:43]

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

[commit-bot: I haz the power, 2016-02-10 15:46:43]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2016-02-10 16:58:58]

Patchset #9 (id:250001) has been deleted

[Mathieu, 2016-02-10 17:01:37]

On 2016/02/10 15:46:43, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

gn should be good now, let's see if we have remaining failures on the GYP side.

[Mathieu, 2016-02-10 17:01:41]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-02-10 17:01:41]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1681663002/#ps270001
(title: "android gn fix")

[commit-bot: I haz the power, 2016-02-10 17:03:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1681663002/270001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1681663002/270001

[commit-bot: I haz the power, 2016-02-10 18:26:35]

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

[commit-bot: I haz the power, 2016-02-10 18:26:35]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2016-02-10 19:37:09]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-02-10 19:37:09]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1681663002/#ps290001
(title: "gn fix!")

[commit-bot: I haz the power, 2016-02-10 19:38:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1681663002/290001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1681663002/290001

[commit-bot: I haz the power, 2016-02-10 21:23:28]

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

[commit-bot: I haz the power, 2016-02-10 21:23:29]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mathieu, 2016-02-10 21:24:38]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2016-02-10 21:34:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1681663002/290001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1681663002/290001

[commit-bot: I haz the power, 2016-02-11 00:23:57]

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

[commit-bot: I haz the power, 2016-02-11 00:23:58]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2016-02-11 00:45:35]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-02-11 00:45:36]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1681663002/#ps310001
(title: "trailing slash")

[Nico, 2016-02-11 00:45:46]

I'm reading quite a bit about swarming and gn recently, and I discovered that gn
doesn't use the .isolate files at all. Instead, tools/mb/mb.py writes the
.isolate files based on the output of `gn desc out/Release //base:base_unittests
runtime_deps` (etc).  For test data, it needs data statements such as
https://code.google.com/p/chromium/codesearch#chromium/src/sql/BUILD.gn&q=tes...

I see that you did this, but I'm guessing your data entry also needs a trailing
'/'.

I think it's a bit weird that you had to define a template for the test, and a
_local target – did you get this form elsewhere? It'd be better if the same
target was used for devs and bots.

[commit-bot: I haz the power, 2016-02-11 00:46:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1681663002/310001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1681663002/310001

[Dirk Pranke, 2016-02-11 01:06:40]

On 2016/02/11 00:45:46, Nico wrote:
> I'm reading quite a bit about swarming and gn recently, and I discovered that
gn
> doesn't use the .isolate files at all. Instead, tools/mb/mb.py writes the
> .isolate files based on the output of `gn desc out/Release
//base:base_unittests
> runtime_deps` (etc).  For test data, it needs data statements such as
>
https://code.google.com/p/chromium/codesearch#chromium/src/sql/BUILD.gn&q=tes...
> 
> I see that you did this, but I'm guessing your data entry also needs a
trailing
> '/'

Yup, this part is correct.

> I think it's a bit weird that you had to define a template for the test, and a
> _local target – did you get this form elsewhere? It'd be better if the
same
> target was used for devs and bots.

I agree with Nico; this is pretty strange. Is there some way we can modify the
source
code to the tests so that we don't need to set this define, or we can make it
work
in some way that isn't so dependent on the current working directory?

[Mathieu, 2016-02-11 01:22:25]

On 2016/02/11 01:06:40, Dirk Pranke wrote:
> On 2016/02/11 00:45:46, Nico wrote:
> > I'm reading quite a bit about swarming and gn recently, and I discovered
that
> gn
> > doesn't use the .isolate files at all. Instead, tools/mb/mb.py writes the
> > .isolate files based on the output of `gn desc out/Release
> //base:base_unittests
> > runtime_deps` (etc).  For test data, it needs data statements such as
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/sql/BUILD.gn&q=tes...
> > 
> > I see that you did this, but I'm guessing your data entry also needs a
> trailing
> > '/'
> 
> Yup, this part is correct.
> 
> > I think it's a bit weird that you had to define a template for the test, and
a
> > _local target – did you get this form elsewhere? It'd be better if the
> same
> > target was used for devs and bots.
> 
> I agree with Nico; this is pretty strange. Is there some way we can modify the
> source
> code to the tests so that we don't need to set this define, or we can make it
> work
> in some way that isn't so dependent on the current working directory?

The code [1] uses a define (TEST_DATA_DIR) that indicates where to find the test
data relative to the CWD. Given the CWD is not the same when running locally (in
the checkout) and when running from isolate (third_party/libaddressinput),
Rouslan and I thought this was a pretty good compromise. It's unlikely we will
get the open source project to change how they do things.

[1]
https://github.com/googlei18n/libaddressinput/blob/5eeeb797e79fa01503fcdcbebd...

[Nico, 2016-02-11 01:26:00]

How do you know what the cwd is when running locally? Do you just assume
people run it form src/? (That's bad, since some folks live in
src/out/Release.) Or do you pass an absolute path to that define? (That's
bad 'cause it means that builds on different computes can never produce
identical binaries, which eventually will be bad for test result caching.)

If upstream won't accept patches for this, maybe we can have a downstream
diff?

I think it's ok to land this patch as-is as that part isn't getting worse
here and it allows us to run the thing on swarming, but I think that's
something that should be figured out independently of this patch.

On Wed, Feb 10, 2016 at 8:22 PM, <mathp@chromium.org> wrote:

>
> On 2016/02/11 01:06:40, Dirk Pranke wrote:
> > On 2016/02/11 00:45:46, Nico wrote:
> > > I'm reading quite a bit about swarming and gn recently, and I discovered
> that
> > gn
> > > doesn't use the .isolate files at all. Instead, tools/mb/mb.py writes the
> > > .isolate files based on the output of `gn desc out/Release
> > //base:base_unittests
> > > runtime_deps` (etc).  For test data, it needs data statements such as
> > >
>
>https://code.google.com/p/chromium/codesearch#chromium/src/sql/BUILD.gn&q=test.?data%20file:%5C.gn&sq=package:chromium&type=cs&l=91
> > >
> > > I see that you did this, but I'm guessing your data entry also needs a
> > trailing
> > > '/'
> >
> > Yup, this part is correct.
> >
> > > I think it's a bit weird that you had to define a template for the test,
and
> a
> > > _local target –&nbsp;did you get this form elsewhere? It'd be better if
the
> > same
> > > target was used for devs and bots.
> >
> > I agree with Nico; this is pretty strange. Is there some way we can modify
the
> > source
> > code to the tests so that we don't need to set this define, or we can make
it
> > work
> > in some way that isn't so dependent on the current working directory?
>
> The code [1] uses a define (TEST_DATA_DIR) that indicates where to find the
test
> data relative to the CWD. Given the CWD is not the same when running locally
(in
> the checkout) and when running from isolate (third_party/libaddressinput),
> Rouslan and I thought this was a pretty good compromise. It's unlikely we
will
> get the open source project to change how they do things.
>
>
[1]https://github.com/googlei18n/libaddressinput/blob/5eeeb797e79fa01503fcdcbebdc50036fac023ef/cpp/test/testdata_source.cc#L38
>
> https://codereview.chromium.org/1681663002/
>
>

-- 
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.

[Dirk Pranke, 2016-02-11 01:30:26]

+1 to what Nico wrote.

In fact, on the GN-based bots, the isolate will actually run with
out/{build_dir} 
as the current working directory, so you can't even assume that 
third_party/libaddressinput is the CWD.

If you can't fix this in the source code, I would change the build file to
assume that 
out/{build_dir} is the CWD and add a separate wrapper script that will cd to
that 
directory and run the tests.

But you don't necessarily need to make those changes in this release. I would
probably however only define the targets as needed by the bots, and not provide
a local equivalent.

[Mathieu, 2016-02-11 01:45:17]

On 2016/02/11 01:30:26, Dirk Pranke wrote:
> +1 to what Nico wrote.
> 
> In fact, on the GN-based bots, the isolate will actually run with
> out/{build_dir} 
> as the current working directory, so you can't even assume that 
> third_party/libaddressinput is the CWD.
> 
> If you can't fix this in the source code, I would change the build file to
> assume that 
> out/{build_dir} is the CWD and add a separate wrapper script that will cd to
> that 
> directory and run the tests.
> 
> But you don't necessarily need to make those changes in this release. I would
> probably however only define the targets as needed by the bots, and not
provide
> a local equivalent.

Thanks, those are all good suggestions to solve this weird situation. I will
follow up with changes to address this.

[commit-bot: I haz the power, 2016-02-11 01:53:49]

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

[commit-bot: I haz the power, 2016-02-11 01:53:49]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2016-02-11 15:15:51]

On 2016/02/11 01:53:49, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

I tried mapping src/testdata/ and src/testdata/countryinfo.txt in BUILD.gn
(patchsets 11 and 12) and both fail on linux_chromium_rel_ng (GN bot). 

I have difficulty reproducing the GN case on my machine:

:~/chrome/src$ rm -rf out/Release/*
:~/chrome/src$ gn gen out/Release
Done. Wrote 4323 targets from 894 files in 890ms
:~/chrome/src$ ninja -C out/Release libaddressinput_unittests_run
ninja: Entering directory `out/Release'
[2559/2559] STAMP
obj/third_party/libaddressinput/libaddressinput_unittests_run.stamp
:~/chrome/src$ ./tools/swarming_client/isolate.py run -v -s
out/Release/libaddressinput_unittests.isolated
9335 2016-02-11 15:13:39.041 W: Failed to load /<my
home...>/chrome/src/out/Release/libaddressinput_unittests.isolated.state: [Errno
2] No such file or directory: u'/<my
home...>/chrome/src/out/Release/libaddressinput_unittests.isolated.state'
Execution failure: A .isolate file is required.

[Nico, 2016-02-11 15:22:30]

I believe the _run targets aren't used with gn. Try something like `python
tools\mb\mb.py run //out/yourgnbuilddir libaddressinput_unittests`

[Mathieu, 2016-02-11 16:04:38]

On 2016/02/11 15:22:30, Nico wrote:
> I believe the _run targets aren't used with gn. Try something like `python
> tools\mb\mb.py run //out/yourgnbuilddir libaddressinput_unittests`

Thanks Nico! Indeed, for completeness the command ended up being:

python tools/mb/mb.py run //out/Default libaddressinput_unittests

[Mathieu, 2016-02-11 16:04:43]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-02-11 16:04:43]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1681663002/#ps350001
(title: "gn TEST_DATA_DIR")

[commit-bot: I haz the power, 2016-02-11 16:06:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1681663002/350001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1681663002/350001

[commit-bot: I haz the power, 2016-02-11 19:28:14]

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

[commit-bot: I haz the power, 2016-02-11 19:28:15]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

Transient error: No JSON object could be decoded

[commit-bot: I haz the power, 2016-02-11 19:30:33]

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

[Mathieu, 2016-02-11 19:30:55]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2016-02-11 19:35:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1681663002/350001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1681663002/350001

[commit-bot: I haz the power, 2016-02-11 19:45:48]

Description was changed from

==========
[Testing] Run libaddressinput_unittests on bots

BUG=585151
TEST=bots
==========

to

==========
[Testing] Run libaddressinput_unittests on bots

BUG=585151
TEST=bots
==========

[commit-bot: I haz the power, 2016-02-11 19:45:49]

Committed patchset #13 (id:350001)

[Dirk Pranke, 2016-02-12 23:51:06]

https://codereview.chromium.org/1681663002/diff/350001/third_party/libaddress...
File third_party/libaddressinput/libaddressinput.gyp (right):

https://codereview.chromium.org/1681663002/diff/350001/third_party/libaddress...
third_party/libaddressinput/libaddressinput.gyp:126:
'TEST_DATA_DIR="src/third_party/libaddressinput/src/testdata/"',
this caused TEST_DATA_DIR to grow a trailing slash, which caused the file path
to expand to "src/testdata//countryinfo.txt", which is not so good.

[Dirk Pranke, 2016-02-12 23:51:40]

A revert of this CL (patchset #13 id:350001) has been created in
https://codereview.chromium.org/1693813003/ by dpranke@chromium.org.

The reason for reverting is: Reverting, this caused the test to fail on the Dr
Memory bots (where it was running fine prior to this change).

https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20(D....

[lazyboy, 2016-02-13 00:35:31]

On 2016/02/12 23:51:40, Dirk Pranke wrote:
> A revert of this CL (patchset #13 id:350001) has been created in
> https://codereview.chromium.org/1693813003/ by mailto:dpranke@chromium.org.
> 
> The reason for reverting is: Reverting, this caused the test to fail on the Dr
> Memory bots (where it was running fine prior to this change).
> 
>
https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20(D....

FYI, this is also causing failure on the bot: "Mac 10.10 Tests"
https://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests?numbuilds=50
e.g.
https://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests/builds/53...

[Mathieu, 2016-02-13 05:22:08]

On 2016/02/13 00:35:31, lazyboy wrote:
> On 2016/02/12 23:51:40, Dirk Pranke wrote:
> > A revert of this CL (patchset #13 id:350001) has been created in
> > https://codereview.chromium.org/1693813003/ by mailto:dpranke@chromium.org.
> > 
> > The reason for reverting is: Reverting, this caused the test to fail on the
Dr
> > Memory bots (where it was running fine prior to this change).
> > 
> >
>
https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20(D....
> 
> FYI, this is also causing failure on the bot: "Mac 10.10 Tests"
>
https://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests?numbuilds=50
> e.g.
>
https://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests/builds/53...

Hmm, re Mac 10.10 Tests, it seems as though it's configured for swarming but
it's NOT running on swarming. Since my GYP binary

[Mathieu, 2016-02-13 05:23:10]

On 2016/02/13 05:22:08, Mathieu Perreault wrote:
> On 2016/02/13 00:35:31, lazyboy wrote:
> > On 2016/02/12 23:51:40, Dirk Pranke wrote:
> > > A revert of this CL (patchset #13 id:350001) has been created in
> > > https://codereview.chromium.org/1693813003/ by
mailto:dpranke@chromium.org.
> > > 
> > > The reason for reverting is: Reverting, this caused the test to fail on
the
> Dr
> > > Memory bots (where it was running fine prior to this change).
> > > 
> > >
> >
>
https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20(D....
> > 
> > FYI, this is also causing failure on the bot: "Mac 10.10 Tests"
> >
>
https://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests?numbuilds=50
> > e.g.
> >
>
https://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests/builds/53...
> 
> Hmm, re Mac 10.10 Tests, it seems as though it's configured for swarming but
> it's NOT running on swarming. Since my GYP binary

since my GYP binary (libaddressinput_unittests) only supports a path relative to
the swarm out dir, it's gets complicated! I didn't know bots could decide
whether they swarm or not.

[commit-bot: I haz the power, 2016-02-16 22:36:30]

Description was changed from

==========
[Testing] Run libaddressinput_unittests on bots

BUG=585151
TEST=bots
==========

to

==========
[Testing] Run libaddressinput_unittests on bots

BUG=585151
TEST=bots

Committed: https://crrev.com/6ddda6d1f93fddbe7656dd237344f5f571df8da2
Cr-Commit-Position: refs/heads/master@{#374933}
==========

[commit-bot: I haz the power, 2016-02-16 22:36:31]

Patchset 13 (id:??) landed as
https://crrev.com/6ddda6d1f93fddbe7656dd237344f5f571df8da2
Cr-Commit-Position: refs/heads/master@{#374933}