Move webrtc/api/androidtests to webrtc/sdk/android/instrumentationtests

Move webrtc/api/androidtests to webrtc/sdk/android/instrumentationtests

BUG=webrtc:5882
NOPRESUBMIT=True

Committed: https://crrev.com/665bc3c7ada5341f5461780486e9117d45aa6947
Cr-Commit-Position: refs/heads/master@{#15352}

[magjed_webrtc, 2016-11-30 13:34:58]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 13:35:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-11-30 13:37:15]

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

[commit-bot: I haz the power, 2016-11-30 13:37:15]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/19083)
  android_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/18904)

[magjed_webrtc, 2016-11-30 13:51:38]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 13:51:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[magjed_webrtc, 2016-11-30 13:51:46]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2016-11-30 13:52:50]

Description was changed from

==========
Move webrtc/api/androidtests to webrtc/sdk/android/instrumentationtests

BUG=webrtc:5882
==========

to

==========
Move webrtc/api/androidtests to webrtc/sdk/android/instrumentationtests

BUG=webrtc:5882
==========

[magjed_webrtc, 2016-11-30 13:52:50]

magjed@webrtc.org changed reviewers:
+ kjellander@webrtc.org, sakal@webrtc.org

[commit-bot: I haz the power, 2016-11-30 14:10:51]

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

[commit-bot: I haz the power, 2016-11-30 14:10:51]

Dry run: This issue passed the CQ dry run.

[sakal, 2016-11-30 14:21:30]

https://codereview.webrtc.org/2541823002/diff/20001/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/2541823002/diff/20001/webrtc/BUILD.gn#newcode312
webrtc/BUILD.gn:312:
"sdk/android/instrumentationtests:libjingle_peerconnection_android_unittest",
nit: I think we should have an absolute path here.

Other deps should always use fully-qualified path names unless relative ones are
required for some reason.

https://codereview.webrtc.org/2541823002/diff/20001/webrtc/sdk/android/instru...
File webrtc/sdk/android/instrumentationtests/BUILD.gn (right):

https://codereview.webrtc.org/2541823002/diff/20001/webrtc/sdk/android/instru...
webrtc/sdk/android/instrumentationtests/BUILD.gn:14:
instrumentation_test_apk("libjingle_peerconnection_android_unittest") {
Most BUILD files should have a target with the same name of the directory. This
target should be the first target.

However, it might not be the right thing to rename this to the directory name:
Executables and tests should be given a globally unique name. Technically only
the output names must be unique, but since only the output names appear in the
shell and on bots, it's much less confusing if the name matches the other places
the executable appears.

So back to to groups?

[kjellander_webrtc, 2016-11-30 14:37:08]

https://codereview.webrtc.org/2541823002/diff/20001/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/2541823002/diff/20001/webrtc/BUILD.gn#newcode312
webrtc/BUILD.gn:312:
"sdk/android/instrumentationtests:libjingle_peerconnection_android_unittest",
On 2016/11/30 14:21:30, sakal wrote:
> nit: I think we should have an absolute path here.

Yes, inside if (!build_with_chromium) conditions we can have absolute paths, but
not elsewhere. I don't think we have any recommendations for either though, and
there's probably a mix currently in our codebase.
 
> Other deps should always use fully-qualified path names unless relative ones
are
> required for some reason.

https://codereview.webrtc.org/2541823002/diff/20001/webrtc/sdk/android/instru...
File webrtc/sdk/android/instrumentationtests/BUILD.gn (right):

https://codereview.webrtc.org/2541823002/diff/20001/webrtc/sdk/android/instru...
webrtc/sdk/android/instrumentationtests/BUILD.gn:14:
instrumentation_test_apk("libjingle_peerconnection_android_unittest") {
On 2016/11/30 14:21:30, sakal wrote:
> Most BUILD files should have a target with the same name of the directory.
This
> target should be the first target.
> 
> However, it might not be the right thing to rename this to the directory name:
> Executables and tests should be given a globally unique name. Technically only
> the output names must be unique, but since only the output names appear in the
> shell and on bots, it's much less confusing if the name matches the other
places
> the executable appears.
> 
> So back to to groups?

I agree we shouldn't have neither 'android' or 'instrumentationtests' as group
names.
I suggest just put this into the webrtc/sdk/BUILD.gn file instead.

[sakal, 2016-11-30 14:40:45]

On 2016/11/30 14:37:08, kjellander_webrtc wrote:
> https://codereview.webrtc.org/2541823002/diff/20001/webrtc/BUILD.gn
> File webrtc/BUILD.gn (right):
> 
> https://codereview.webrtc.org/2541823002/diff/20001/webrtc/BUILD.gn#newcode312
> webrtc/BUILD.gn:312:
> "sdk/android/instrumentationtests:libjingle_peerconnection_android_unittest",
> On 2016/11/30 14:21:30, sakal wrote:
> > nit: I think we should have an absolute path here.
> 
> Yes, inside if (!build_with_chromium) conditions we can have absolute paths,
but
> not elsewhere. I don't think we have any recommendations for either though,
and
> there's probably a mix currently in our codebase.
>  
> > Other deps should always use fully-qualified path names unless relative ones
> are
> > required for some reason.
> 
>
https://codereview.webrtc.org/2541823002/diff/20001/webrtc/sdk/android/instru...
> File webrtc/sdk/android/instrumentationtests/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2541823002/diff/20001/webrtc/sdk/android/instru...
> webrtc/sdk/android/instrumentationtests/BUILD.gn:14:
> instrumentation_test_apk("libjingle_peerconnection_android_unittest") {
> On 2016/11/30 14:21:30, sakal wrote:
> > Most BUILD files should have a target with the same name of the directory.
> This
> > target should be the first target.
> > 
> > However, it might not be the right thing to rename this to the directory
name:
> > Executables and tests should be given a globally unique name. Technically
only
> > the output names must be unique, but since only the output names appear in
the
> > shell and on bots, it's much less confusing if the name matches the other
> places
> > the executable appears.
> > 
> > So back to to groups?
> 
> I agree we shouldn't have neither 'android' or 'instrumentationtests' as group
> names.
> I suggest just put this into the webrtc/sdk/BUILD.gn file instead.

In case it is not clear, my pastes were from GN style guide:
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...

It also says:
It usually makes sense to have more build files closer to the code than fewer
ones at the toplevel (this is in contrast with what we did with GYP). This makes
things easier to find and owners reviews easier since changes are more focused.

I don't think we should mix Android and iOS BUILD files because the code bases
are separate.

[magjed_webrtc, 2016-11-30 15:11:49]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 15:11:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-11-30 15:13:25]

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

[commit-bot: I haz the power, 2016-11-30 15:13:26]

Dry run: This issue passed the CQ dry run.

[magjed_webrtc, 2016-11-30 15:15:36]

https://codereview.webrtc.org/2541823002/diff/20001/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/2541823002/diff/20001/webrtc/BUILD.gn#newcode312
webrtc/BUILD.gn:312:
"sdk/android/instrumentationtests:libjingle_peerconnection_android_unittest",
On 2016/11/30 14:21:30, sakal wrote:
> nit: I think we should have an absolute path here.
> 
> Other deps should always use fully-qualified path names unless relative ones
are
> required for some reason.

Done.

https://codereview.webrtc.org/2541823002/diff/20001/webrtc/sdk/android/instru...
File webrtc/sdk/android/instrumentationtests/BUILD.gn (right):

https://codereview.webrtc.org/2541823002/diff/20001/webrtc/sdk/android/instru...
webrtc/sdk/android/instrumentationtests/BUILD.gn:14:
instrumentation_test_apk("libjingle_peerconnection_android_unittest") {
On 2016/11/30 14:37:08, kjellander_webrtc wrote:
> On 2016/11/30 14:21:30, sakal wrote:
> > Most BUILD files should have a target with the same name of the directory.
> This
> > target should be the first target.
> > 
> > However, it might not be the right thing to rename this to the directory
name:
> > Executables and tests should be given a globally unique name. Technically
only
> > the output names must be unique, but since only the output names appear in
the
> > shell and on bots, it's much less confusing if the name matches the other
> places
> > the executable appears.
> > 
> > So back to to groups?
> 
> I agree we shouldn't have neither 'android' or 'instrumentationtests' as group
> names.
> I suggest just put this into the webrtc/sdk/BUILD.gn file instead. 

I would also prefer to have smaller BUILD files closer to the code so it's more
modular, but I'm a GN noob so I don't know the pros and cons of this.

[kjellander_webrtc, 2016-11-30 18:41:54]

lgtm

https://codereview.webrtc.org/2541823002/diff/20001/webrtc/sdk/android/instru...
File webrtc/sdk/android/instrumentationtests/BUILD.gn (right):

https://codereview.webrtc.org/2541823002/diff/20001/webrtc/sdk/android/instru...
webrtc/sdk/android/instrumentationtests/BUILD.gn:14:
instrumentation_test_apk("libjingle_peerconnection_android_unittest") {
On 2016/11/30 15:15:36, magjed_webrtc wrote:
> On 2016/11/30 14:37:08, kjellander_webrtc wrote:
> > On 2016/11/30 14:21:30, sakal wrote:
> > > Most BUILD files should have a target with the same name of the directory.
> > This
> > > target should be the first target.
> > > 
> > > However, it might not be the right thing to rename this to the directory
> name:
> > > Executables and tests should be given a globally unique name. Technically
> only
> > > the output names must be unique, but since only the output names appear in
> the
> > > shell and on bots, it's much less confusing if the name matches the other
> > places
> > > the executable appears.
> > > 
> > > So back to to groups?
> > 
> > I agree we shouldn't have neither 'android' or 'instrumentationtests' as
group
> > names.
> > I suggest just put this into the webrtc/sdk/BUILD.gn file instead. 
> 
> I would also prefer to have smaller BUILD files closer to the code so it's
more
> modular, but I'm a GN noob so I don't know the pros and cons of this.

I can't think of any significant pros and cons so please go ahead with this.

[sakal, 2016-12-01 08:36:08]

Shouldn't we add owners to the new directories?

https://codereview.webrtc.org/2541823002/diff/40001/webrtc/sdk/android/instru...
File webrtc/sdk/android/instrumentationtests/BUILD.gn (right):

https://codereview.webrtc.org/2541823002/diff/40001/webrtc/sdk/android/instru...
webrtc/sdk/android/instrumentationtests/BUILD.gn:9:
import("../../../build/webrtc.gni")
nit: I think we could use fully qualified name for this as well since this is
not build from chromium. I find it nicer and style guide says we should do it.

[magjed_webrtc, 2016-12-01 08:55:18]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 08:55:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[magjed_webrtc, 2016-12-01 08:56:40]

You (sakal) and me are owners of the whole /webrtc/sdk/android folder.

https://codereview.webrtc.org/2541823002/diff/40001/webrtc/sdk/android/instru...
File webrtc/sdk/android/instrumentationtests/BUILD.gn (right):

https://codereview.webrtc.org/2541823002/diff/40001/webrtc/sdk/android/instru...
webrtc/sdk/android/instrumentationtests/BUILD.gn:9:
import("../../../build/webrtc.gni")
On 2016/12/01 08:36:08, sakal wrote:
> nit: I think we could use fully qualified name for this as well since this is
> not build from chromium. I find it nicer and style guide says we should do it.

Done.

[sakal, 2016-12-01 09:08:04]

lgtm

[commit-bot: I haz the power, 2016-12-01 09:16:44]

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

[commit-bot: I haz the power, 2016-12-01 09:16:45]

Dry run: This issue passed the CQ dry run.

[magjed_webrtc, 2016-12-01 09:34:21]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2016-12-01 09:34:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2541823002/#ps60001 (title:
"Make include for webrtc.gni absolute")

[commit-bot: I haz the power, 2016-12-01 09:34:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-01 09:37:22]

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

[commit-bot: I haz the power, 2016-12-01 09:37:23]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10913)

[magjed_webrtc, 2016-12-01 09:40:55]

Description was changed from

==========
Move webrtc/api/androidtests to webrtc/sdk/android/instrumentationtests

BUG=webrtc:5882
==========

to

==========
Move webrtc/api/androidtests to webrtc/sdk/android/instrumentationtests

BUG=webrtc:5882
NOPRESUBMIT=True
==========

[magjed_webrtc, 2016-12-01 09:44:56]

Presubmit is failing because I'm removing the OWNERS file of the files I'm
owning.

Why isn't the owners check applied before changes to owners files are made? I.e.
it's weird that you can add yourself to an OWNER file without owner approval,
but you can't remove yourself from an owner file without approval.

[magjed_webrtc, 2016-12-01 09:45:03]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2016-12-01 09:45:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-01 09:45:32]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480585503664170,
"parent_rev": "216df5e155198a1d4c84fffc6ff28ab1a22a10a3", "commit_rev":
"71ec582684a24b789e022340ddc3baadb18f7c05"}

[commit-bot: I haz the power, 2016-12-01 09:45:35]

Description was changed from

==========
Move webrtc/api/androidtests to webrtc/sdk/android/instrumentationtests

BUG=webrtc:5882
NOPRESUBMIT=True
==========

to

==========
Move webrtc/api/androidtests to webrtc/sdk/android/instrumentationtests

BUG=webrtc:5882
NOPRESUBMIT=True
==========

[commit-bot: I haz the power, 2016-12-01 09:45:36]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-12-01 09:45:40]

Description was changed from

==========
Move webrtc/api/androidtests to webrtc/sdk/android/instrumentationtests

BUG=webrtc:5882
NOPRESUBMIT=True
==========

to

==========
Move webrtc/api/androidtests to webrtc/sdk/android/instrumentationtests

BUG=webrtc:5882
NOPRESUBMIT=True

Committed: https://crrev.com/665bc3c7ada5341f5461780486e9117d45aa6947
Cr-Commit-Position: refs/heads/master@{#15352}
==========

[commit-bot: I haz the power, 2016-12-01 09:45:41]

Patchset 3 (id:??) landed as
https://crrev.com/665bc3c7ada5341f5461780486e9117d45aa6947
Cr-Commit-Position: refs/heads/master@{#15352}