Moving mojo/validation test into LayoutTests

Moving mojo/validation test into LayoutTests

part of the move started in Issue 2744763002
This moves the validation test

BUG=647036

[damargulis, 2017-03-14 18:55:20]

damargulis@chromium.org changed reviewers:
+ alokp@chromium.org, jbroman@chromium.org

[damargulis, 2017-03-14 18:55:21]

Hi,

This is the beginning of the work to move the validation test from
mojo/public/js/tests into WebKit, under third_party/WebKit/LayoutTests/mojo.

I have run into trouble however because I no longer have access to the file
module.  I wrote a comment in the code at the location I am having issues, and
any advice or ideas you have would be greatly appreciated!

Thanks,
Dan

https://codereview.chromium.org/2745293005/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/mojo/validation.html (right):

https://codereview.chromium.org/2745293005/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/mojo/validation.html:141: var sourceRoot =
file.getSourceRootDirectory();
Since I am moving the location of this test, I no longer have access to the file
module, which is implemented in c++.  How can I replicate this behavior
(basically getting a directory, and reading all files contained within it into a
string) in javascript.  Normally javascript wouldn't have access to the
filesystem for security reasons, but is there a way to get around that for
testing purposes?
Thanks!

[yzshen1, 2017-03-14 19:58:58]

On 2017/03/14 18:55:21, damargulis wrote:
> Hi,
> 
> This is the beginning of the work to move the validation test from
> mojo/public/js/tests into WebKit, under third_party/WebKit/LayoutTests/mojo.
> 
> I have run into trouble however because I no longer have access to the file
> module.  I wrote a comment in the code at the location I am having issues, and
> any advice or ideas you have would be greatly appreciated!
> 
> Thanks,
> Dan
> 
>
https://codereview.chromium.org/2745293005/diff/1/third_party/WebKit/LayoutTe...
> File third_party/WebKit/LayoutTests/mojo/validation.html (right):
> 
>
https://codereview.chromium.org/2745293005/diff/1/third_party/WebKit/LayoutTe...
> third_party/WebKit/LayoutTests/mojo/validation.html:141: var sourceRoot =
> file.getSourceRootDirectory();
> Since I am moving the location of this test, I no longer have access to the
file
> module, which is implemented in c++.  How can I replicate this behavior
> (basically getting a directory, and reading all files contained within it into
a
> string) in javascript.  Normally javascript wouldn't have access to the
> filesystem for security reasons, but is there a way to get around that for
> testing purposes?
> Thanks!

(jbroman and alokp may have better ideas.)

I haven't tried myself, but maybe fetching file urls works? A lot of layout
tests are not run using HTTP, so I guess maybe that works.
The last resort is to expose some testing APIs to layout tests.

[jbroman, 2017-03-14 21:22:16]

https://codereview.chromium.org/2745293005/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/mojo/validation.html (right):

https://codereview.chromium.org/2745293005/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/mojo/validation.html:141: var sourceRoot =
file.getSourceRootDirectory();
On 2017/03/14 at 18:55:21, damargulis wrote:
> Since I am moving the location of this test, I no longer have access to the
file module, which is implemented in c++.  How can I replicate this behavior
(basically getting a directory, and reading all files contained within it into a
string) in javascript.  Normally javascript wouldn't have access to the
filesystem for security reasons, but is there a way to get around that for
testing purposes?
> Thanks!

During layout tests, it is possible to fetch a file URL from a file URL (even
though this is normally disallowed). You can use that to fetch an individual
file, but I don't believe we offer a way to enumerate directory contents. But
this will allow you to fetch stuff inside LayoutTests/ (and it works the same
over http and file URLs).

The alternative would be to express this as a Blink unit test, in which case you
can use blink::testing::readFromFile.

[damargulis, 2017-03-15 00:49:36]

On 2017/03/14 21:22:16, jbroman wrote:
>
https://codereview.chromium.org/2745293005/diff/1/third_party/WebKit/LayoutTe...
> File third_party/WebKit/LayoutTests/mojo/validation.html (right):
> 
>
https://codereview.chromium.org/2745293005/diff/1/third_party/WebKit/LayoutTe...
> third_party/WebKit/LayoutTests/mojo/validation.html:141: var sourceRoot =
> file.getSourceRootDirectory();
> On 2017/03/14 at 18:55:21, damargulis wrote:
> > Since I am moving the location of this test, I no longer have access to the
> file module, which is implemented in c++.  How can I replicate this behavior
> (basically getting a directory, and reading all files contained within it into
a
> string) in javascript.  Normally javascript wouldn't have access to the
> filesystem for security reasons, but is there a way to get around that for
> testing purposes?
> > Thanks!
> 
> During layout tests, it is possible to fetch a file URL from a file URL (even
> though this is normally disallowed). You can use that to fetch an individual
> file, but I don't believe we offer a way to enumerate directory contents. But
> this will allow you to fetch stuff inside LayoutTests/ (and it works the same
> over http and file URLs).
> 
> The alternative would be to express this as a Blink unit test, in which case
you
> can use blink::testing::readFromFile.

Can you elaborate on this a little more?  I can't seem to get fetch to work
properly. 
Since there is no way to read the directory, I put the directory listing into a
txt file called data_files.txt under
third_party/WebKit/LayoutTests/mojo/resources/data_files.txt.
Requesting using fetch("/resources/data_files.txt") results in an error: Fetch
API cannot load. URL scheme "file" is not supported.  I was eventually able to
get a fetch to work by using localhost i.e.-
fetch("http://localhost:8000/resources/data_files.txt") - but every request
results in a 404.  I've tried basically every possible url I can think to try,
but everything results in a 404.  
I would like to avoid rewritting this as a Blink test in order to keep all of
these tests consistent. Any ideas?

[jbroman, 2017-03-15 14:47:46]

On 2017/03/15 at 00:49:36, damargulis wrote:
> On 2017/03/14 21:22:16, jbroman wrote:
> >
https://codereview.chromium.org/2745293005/diff/1/third_party/WebKit/LayoutTe...
> > File third_party/WebKit/LayoutTests/mojo/validation.html (right):
> > 
> >
https://codereview.chromium.org/2745293005/diff/1/third_party/WebKit/LayoutTe...
> > third_party/WebKit/LayoutTests/mojo/validation.html:141: var sourceRoot =
> > file.getSourceRootDirectory();
> > On 2017/03/14 at 18:55:21, damargulis wrote:
> > > Since I am moving the location of this test, I no longer have access to
the
> > file module, which is implemented in c++.  How can I replicate this behavior
> > (basically getting a directory, and reading all files contained within it
into a
> > string) in javascript.  Normally javascript wouldn't have access to the
> > filesystem for security reasons, but is there a way to get around that for
> > testing purposes?
> > > Thanks!
> > 
> > During layout tests, it is possible to fetch a file URL from a file URL
(even
> > though this is normally disallowed). You can use that to fetch an individual
> > file, but I don't believe we offer a way to enumerate directory contents.
But
> > this will allow you to fetch stuff inside LayoutTests/ (and it works the
same
> > over http and file URLs).
> > 
> > The alternative would be to express this as a Blink unit test, in which case
you
> > can use blink::testing::readFromFile.
> 
> Can you elaborate on this a little more?  I can't seem to get fetch to work
properly. 
> Since there is no way to read the directory, I put the directory listing into
a txt file called data_files.txt under
third_party/WebKit/LayoutTests/mojo/resources/data_files.txt.
> Requesting using fetch("/resources/data_files.txt") results in an error: Fetch
API cannot load. URL scheme "file" is not supported.  I was eventually able to
get a fetch to work by using localhost i.e.-
fetch("http://localhost:8000/resources/data_files.txt") - but every request
results in a 404.  I've tried basically every possible url I can think to try,
but everything results in a 404.  
> I would like to avoid rewritting this as a Blink test in order to keep all of
these tests consistent. Any ideas?

fetch won't work for file: URLs, but I think XMLHttpRequest will (in layout
tests). Something like this should give you a similar effect to fetch:

function fetchLite(url) {
  return new Promise((resolve, reject) {
    var xhr = new XMLHttpRequest();
    xhr.open('GET', url);
    xhr.onreadystatechange = () => {
      if (xhr.readyState != 4) return;
      if (xhr.status == 200) resolve(xhr.responseText);
      else reject(xhr.responseText);
    };
    xhr.send();
  });
}

[damargulis, 2017-03-15 20:52:06]

On 2017/03/15 14:47:46, jbroman wrote:
> On 2017/03/15 at 00:49:36, damargulis wrote:
> > On 2017/03/14 21:22:16, jbroman wrote:
> > >
>
https://codereview.chromium.org/2745293005/diff/1/third_party/WebKit/LayoutTe...
> > > File third_party/WebKit/LayoutTests/mojo/validation.html (right):
> > > 
> > >
>
https://codereview.chromium.org/2745293005/diff/1/third_party/WebKit/LayoutTe...
> > > third_party/WebKit/LayoutTests/mojo/validation.html:141: var sourceRoot =
> > > file.getSourceRootDirectory();
> > > On 2017/03/14 at 18:55:21, damargulis wrote:
> > > > Since I am moving the location of this test, I no longer have access to
> the
> > > file module, which is implemented in c++.  How can I replicate this
behavior
> > > (basically getting a directory, and reading all files contained within it
> into a
> > > string) in javascript.  Normally javascript wouldn't have access to the
> > > filesystem for security reasons, but is there a way to get around that for
> > > testing purposes?
> > > > Thanks!
> > > 
> > > During layout tests, it is possible to fetch a file URL from a file URL
> (even
> > > though this is normally disallowed). You can use that to fetch an
individual
> > > file, but I don't believe we offer a way to enumerate directory contents.
> But
> > > this will allow you to fetch stuff inside LayoutTests/ (and it works the
> same
> > > over http and file URLs).
> > > 
> > > The alternative would be to express this as a Blink unit test, in which
case
> you
> > > can use blink::testing::readFromFile.
> > 
> > Can you elaborate on this a little more?  I can't seem to get fetch to work
> properly. 
> > Since there is no way to read the directory, I put the directory listing
into
> a txt file called data_files.txt under
> third_party/WebKit/LayoutTests/mojo/resources/data_files.txt.
> > Requesting using fetch("/resources/data_files.txt") results in an error:
Fetch
> API cannot load. URL scheme "file" is not supported.  I was eventually able to
> get a fetch to work by using localhost i.e.-
> fetch("http://localhost:8000/resources/data_files.txt") - but every request
> results in a 404.  I've tried basically every possible url I can think to try,
> but everything results in a 404.  
> > I would like to avoid rewritting this as a Blink test in order to keep all
of
> these tests consistent. Any ideas?
> 
> fetch won't work for file: URLs, but I think XMLHttpRequest will (in layout
> tests). Something like this should give you a similar effect to fetch:
> 
> function fetchLite(url) {
>   return new Promise((resolve, reject) {
>     var xhr = new XMLHttpRequest();
>     xhr.open('GET', url);
>     xhr.onreadystatechange = () => {
>       if (xhr.readyState != 4) return;
>       if (xhr.status == 200) resolve(xhr.responseText);
>       else reject(xhr.responseText);
>     };
>     xhr.send();
>   });
> }

Thanks for your help! The validation tests are now all passing.  Since there is
no way to read the directory, I copied the directory listing into a
data_files.txt file, which is then used to find and fetch all of the test files.
 The major downside to this is that if any data files are added or removed, this
directory file will also need to be updated. However, I am not sure if there is
any way around that.

[jbroman, 2017-03-15 20:59:07]

https://codereview.chromium.org/2745293005/diff/40001/mojo/public/interfaces/...
File
mojo/public/interfaces/bindings/tests/data/validation/associated_conformance_mthd0_good.data
(left):

https://codereview.chromium.org/2745293005/diff/40001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/data/validation/associated_conformance_mthd0_good.data:1:
[dist4]message_header  // num_bytes
Don't the C++ unit tests still rely on these data files?

[damargulis, 2017-03-15 21:03:32]

On 2017/03/15 20:59:07, jbroman wrote:
>
https://codereview.chromium.org/2745293005/diff/40001/mojo/public/interfaces/...
> File
>
mojo/public/interfaces/bindings/tests/data/validation/associated_conformance_mthd0_good.data
> (left):
> 
>
https://codereview.chromium.org/2745293005/diff/40001/mojo/public/interfaces/...
>
mojo/public/interfaces/bindings/tests/data/validation/associated_conformance_mthd0_good.data:1:
> [dist4]message_header  // num_bytes
> Don't the C++ unit tests still rely on these data files?

Whoops, looks like they do.  Putting them back now.

[damargulis, 2017-03-15 21:08:47]

Description was changed from

==========
Moving mojo/validation test into LayoutTests

part of the move started in Issue 2744763002
This moves the validation test

BUG=
==========

to

==========
Moving mojo/validation test into LayoutTests

part of the move started in Issue 2744763002
This moves the validation test

BUG=
==========

[damargulis, 2017-03-15 21:08:47]

damargulis@chromium.org changed reviewers:
+ yzshen@chromium.org

[damargulis, 2017-03-15 22:39:18]

https://codereview.chromium.org/2745293005/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/mojo/validation.html (right):

https://codereview.chromium.org/2745293005/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/mojo/validation.html:144:
resolve(xhr.responseText);
I should mention here, I was not able to use the fetchLite exactly the way you
posted it.  For some reason, every response had a status of 0, even when it
successfully received data.  I'm not sure if this is due to the testing
framework, or if there is other way around the issue besides simply ignoring it.

[yzshen1, 2017-03-16 16:40:54]

https://codereview.chromium.org/2745293005/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/mojo/resources/data/validation/associated_conformance_mthd0_good.data
(right):

https://codereview.chromium.org/2745293005/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/mojo/resources/data/validation/associated_conformance_mthd0_good.data:1:
[dist4]message_header  // num_bytes
Are you going to duplicate the test data? I don't think that is a good idea.
Duplication files in the source tree also lead to confusion and mismatch at the
end.

Could you fetch from the original location?

https://codereview.chromium.org/2745293005/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/mojo/resources/data_files.txt (right):

https://codereview.chromium.org/2745293005/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/mojo/resources/data_files.txt:1:
associated_conformance_mthd0_good.data
It is very easy to forget about updating this file. Especially because
newly-added tests will be silently ignored and it appears to the developer that
everything still passes.

How about generating this file using a GN action and make that a data dependency
of the layout test?

[jbroman, 2017-03-16 19:48:04]

https://codereview.chromium.org/2745293005/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/mojo/resources/data_files.txt (right):

https://codereview.chromium.org/2745293005/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/mojo/resources/data_files.txt:1:
associated_conformance_mthd0_good.data
On 2017/03/16 at 16:40:54, yzshen1 wrote:
> It is very easy to forget about updating this file. Especially because
newly-added tests will be silently ignored and it appears to the developer that
everything still passes.
> 
> How about generating this file using a GN action and make that a data
dependency of the layout test?

The tricky thing here will be that we don't want gn actions writing into the
source tree, and I don't think there's any attempt to overlay this with
$OUTDIR/gen/ when running layout tests. To this point, I think we've simply
duplicated data files, though I understand why that's undesirable here.

While it might work at least on file URLs to just fetch from a directory outside
LayoutTests/, I'm not aware of anything doing that (and you might want to talk
to qyearsley@ or other Blink infra folk to find out whether they'd even be okay
with that). The layout test infra isn't really built under the assumption that
it will need to share data like this, AFAIK.

I'm not sure what the best solution to that is. Blink unit tests can certainly
do it, but I understand the desire not to have a different kind of test for this
reason.

[yzshen1, 2017-03-16 20:51:18]

https://codereview.chromium.org/2745293005/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/mojo/resources/data_files.txt (right):

https://codereview.chromium.org/2745293005/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/mojo/resources/data_files.txt:1:
associated_conformance_mthd0_good.data
On 2017/03/16 19:48:03, jbroman wrote:
> On 2017/03/16 at 16:40:54, yzshen1 wrote:
> > It is very easy to forget about updating this file. Especially because
> newly-added tests will be silently ignored and it appears to the developer
that
> everything still passes.
> > 
> > How about generating this file using a GN action and make that a data
> dependency of the layout test?
> 
> The tricky thing here will be that we don't want gn actions writing into the
> source tree, and I don't think there's any attempt to overlay this with
> $OUTDIR/gen/ when running layout tests. To this point, I think we've simply
> duplicated data files, though I understand why that's undesirable here.

Could we support fetching data from $OUTDIR/gen/?

That is not just necessary for this test. Very soon, I am going to change
loading of mojom.js from using some special AMD define() API to using <script>
tag. It would be nice to be able to fetch files from $OUTDIR/gen/*.

> 
> While it might work at least on file URLs to just fetch from a directory
outside
> LayoutTests/, I'm not aware of anything doing that (and you might want to talk
> to qyearsley@ or other Blink infra folk to find out whether they'd even be
okay
> with that). The layout test infra isn't really built under the assumption that
> it will need to share data like this, AFAIK.
> 
> I'm not sure what the best solution to that is. Blink unit tests can certainly
> do it, but I understand the desire not to have a different kind of test for
this
> reason.

[yzshen1, 2017-03-16 20:53:08]

On 2017/03/16 20:51:18, yzshen1 wrote:
>
https://codereview.chromium.org/2745293005/diff/60001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/mojo/resources/data_files.txt (right):
> 
>
https://codereview.chromium.org/2745293005/diff/60001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/mojo/resources/data_files.txt:1:
> associated_conformance_mthd0_good.data
> On 2017/03/16 19:48:03, jbroman wrote:
> > On 2017/03/16 at 16:40:54, yzshen1 wrote:
> > > It is very easy to forget about updating this file. Especially because
> > newly-added tests will be silently ignored and it appears to the developer
> that
> > everything still passes.
> > > 
> > > How about generating this file using a GN action and make that a data
> > dependency of the layout test?
> > 
> > The tricky thing here will be that we don't want gn actions writing into the
> > source tree, and I don't think there's any attempt to overlay this with
> > $OUTDIR/gen/ when running layout tests. To this point, I think we've simply
> > duplicated data files, though I understand why that's undesirable here.
> 
> Could we support fetching data from $OUTDIR/gen/?
> 
> That is not just necessary for this test. Very soon, I am going to change
> loading of mojom.js from using some special AMD define() API to using <script>
> tag. It would be nice to be able to fetch files from $OUTDIR/gen/*.
> 
> > 
> > While it might work at least on file URLs to just fetch from a directory
> outside
> > LayoutTests/, I'm not aware of anything doing that (and you might want to
talk
> > to qyearsley@ or other Blink infra folk to find out whether they'd even be
> okay
> > with that). The layout test infra isn't really built under the assumption
that
> > it will need to share data like this, AFAIK.
> > 
> > I'm not sure what the best solution to that is. Blink unit tests can
certainly
> > do it, but I understand the desire not to have a different kind of test for
> this
> > reason.

Oh, I realized I didn't finish your whole comment and you actually have answered
my question. I could ping qyearsley about that. Thanks!

[yzshen1, 2017-03-16 22:08:04]

On 2017/03/16 20:53:08, yzshen1 wrote:
> On 2017/03/16 20:51:18, yzshen1 wrote:
> >
>
https://codereview.chromium.org/2745293005/diff/60001/third_party/WebKit/Layo...
> > File third_party/WebKit/LayoutTests/mojo/resources/data_files.txt (right):
> > 
> >
>
https://codereview.chromium.org/2745293005/diff/60001/third_party/WebKit/Layo...
> > third_party/WebKit/LayoutTests/mojo/resources/data_files.txt:1:
> > associated_conformance_mthd0_good.data
> > On 2017/03/16 19:48:03, jbroman wrote:
> > > On 2017/03/16 at 16:40:54, yzshen1 wrote:
> > > > It is very easy to forget about updating this file. Especially because
> > > newly-added tests will be silently ignored and it appears to the developer
> > that
> > > everything still passes.
> > > > 
> > > > How about generating this file using a GN action and make that a data
> > > dependency of the layout test?
> > > 
> > > The tricky thing here will be that we don't want gn actions writing into
the
> > > source tree, and I don't think there's any attempt to overlay this with
> > > $OUTDIR/gen/ when running layout tests. To this point, I think we've
simply
> > > duplicated data files, though I understand why that's undesirable here.
> > 
> > Could we support fetching data from $OUTDIR/gen/?
> > 
> > That is not just necessary for this test. Very soon, I am going to change
> > loading of mojom.js from using some special AMD define() API to using
<script>
> > tag. It would be nice to be able to fetch files from $OUTDIR/gen/*.
> > 
> > > 
> > > While it might work at least on file URLs to just fetch from a directory
> > outside
> > > LayoutTests/, I'm not aware of anything doing that (and you might want to
> talk
> > > to qyearsley@ or other Blink infra folk to find out whether they'd even be
> > okay
> > > with that). The layout test infra isn't really built under the assumption
> that
> > > it will need to share data like this, AFAIK.
> > > 
> > > I'm not sure what the best solution to that is. Blink unit tests can
> certainly
> > > do it, but I understand the desire not to have a different kind of test
for
> > this
> > > reason.
> 
> Oh, I realized I didn't finish your whole comment and you actually have
answered
> my question. I could ping qyearsley about that. Thanks!

I chatted with qyearsley@ and here is the suggestion from him:
"""
Use some special magic path prefix (e.g. "/gen/") to indicate that you want to
look for the file in the output dir, and then inside of content shell when
running layout tests, find the actual output dir and use that.

Related code:
https://cs.chromium.org/chromium/src/content/shell/renderer/layout_test/blink...
https://cs.chromium.org/chromium/src/content/shell/renderer/layout_test/blink...
"""

[yzshen1, 2017-03-16 22:08:35]

yzshen@chromium.org changed reviewers:
+ qyearsley@chromium.org

[damargulis, 2017-03-18 01:22:43]

https://codereview.chromium.org/2745293005/diff/80001/content/shell/renderer/...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/2745293005/diff/80001/content/shell/renderer/...
content/shell/renderer/layout_test/blink_test_runner.cc:378: utf8_url.compare(0,
genPrefixLen, genPrefix, genPrefixLen))
I was able to use the BUILD.gn file to generate a list of data files and output
that file into the /gen/ directory.  However, I am not sure how I can use this
to redirect to the correct directory.  I was able to use this line here to check
if a fetch is attempting to look for a gen file, but I can't seem to find a way
to find the /gen/ directory.  Below, they use
LayoutTestRenderThreadObserver::GetInstance()->webkit_source_dir()
to find the source directory, but I can't seem to find the out or gen directory
saved to any variables anywhere.  

I will keep working on this next week, but let me know if you have any ideas!

[yzshen1, 2017-03-19 05:51:40]

On 2017/03/18 01:22:43, damargulis wrote:
>
https://codereview.chromium.org/2745293005/diff/80001/content/shell/renderer/...
> File content/shell/renderer/layout_test/blink_test_runner.cc (right):
> 
>
https://codereview.chromium.org/2745293005/diff/80001/content/shell/renderer/...
> content/shell/renderer/layout_test/blink_test_runner.cc:378:
utf8_url.compare(0,
> genPrefixLen, genPrefix, genPrefixLen))
> I was able to use the BUILD.gn file to generate a list of data files and
output
> that file into the /gen/ directory.  However, I am not sure how I can use this
> to redirect to the correct directory.  I was able to use this line here to
check
> if a fetch is attempting to look for a gen file, but I can't seem to find a
way
> to find the /gen/ directory.  Below, they use
> LayoutTestRenderThreadObserver::GetInstance()->webkit_source_dir()
> to find the source directory, but I can't seem to find the out or gen
directory
> saved to any variables anywhere.  
> 
> I will keep working on this next week, but let me know if you have any ideas!

Hi, Daniel.

Because I also need the capability of loading files from /gen/, so I went ahead
and implement that support in this CL (under review):
https://codereview.chromium.org/2759803002/

You could depend on that CL and continue your work of moving validation tests.

Thanks!

[damargulis, 2017-03-20 18:37:20]

Description was changed from

==========
Moving mojo/validation test into LayoutTests

part of the move started in Issue 2744763002
This moves the validation test

BUG=
==========

to

==========
Moving mojo/validation test into LayoutTests

part of the move started in Issue 2744763002
This moves the validation test

BUG=
CQ-DEPEND=CL:2759803002
==========

[damargulis, 2017-03-21 16:56:44]

https://codereview.chromium.org/2745293005/diff/120001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2745293005/diff/120001/BUILD.gn#newcode902
BUILD.gn:902: "//third_party/WebKit/LayoutTests/mojo:data_files",
Using your patch, I was able to finish the validation tests by using the
generated file directory.  However, this was my first time working with .gn
files, so I am not sure if I organized the BUILD directories in the best way.  

Also, I marked this change as dependent by including CQ-DEPEND=CL:2759803002 in
the description, but it is unclear to me if this is being read properly by the
system.  Let me know if I need to add anything else, or any other changes I
should make.

Thanks for the help!

[damargulis, 2017-03-21 16:57:03]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 16:57:43]

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

[commit-bot: I haz the power, 2017-03-21 17:01:04]

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

[commit-bot: I haz the power, 2017-03-21 17:01:05]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[damargulis, 2017-03-21 19:13:11]

Description was changed from

==========
Moving mojo/validation test into LayoutTests

part of the move started in Issue 2744763002
This moves the validation test

BUG=
CQ-DEPEND=CL:2759803002
==========

to

==========
Moving mojo/validation test into LayoutTests

part of the move started in Issue 2744763002
This moves the validation test

BUG=647036
CQ-DEPEND=CL:2759803002
==========

[damargulis, 2017-03-21 19:42:44]

Description was changed from

==========
Moving mojo/validation test into LayoutTests

part of the move started in Issue 2744763002
This moves the validation test

BUG=647036
CQ-DEPEND=CL:2759803002
==========

to

==========
Moving mojo/validation test into LayoutTests

part of the move started in Issue 2744763002
This moves the validation test

BUG=647036
==========

[damargulis, 2017-03-21 21:52:40]

On 2017/03/21 16:56:44, damargulis wrote:
> https://codereview.chromium.org/2745293005/diff/120001/BUILD.gn
> File BUILD.gn (right):
> 
> https://codereview.chromium.org/2745293005/diff/120001/BUILD.gn#newcode902
> BUILD.gn:902: "//third_party/WebKit/LayoutTests/mojo:data_files",
> Using your patch, I was able to finish the validation tests by using the
> generated file directory.  However, this was my first time working with .gn
> files, so I am not sure if I organized the BUILD directories in the best way. 

> 
> Also, I marked this change as dependent by including CQ-DEPEND=CL:2759803002
in
> the description, but it is unclear to me if this is being read properly by the
> system.  Let me know if I need to add anything else, or any other changes I
> should make.
> 
> Thanks for the help!

After speaking with alokp, I reorganized the BUILD so that all of the data stays
within mojo/public/interfaces/bindings/tests, which should be a more logical
flow.  I was also able to get this CL to depend on yzshen1's CL.  I believe this
test is working properly now, but let me know what other changes I should make!

[damargulis, 2017-03-21 22:37:46]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 22:38:29]

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

[commit-bot: I haz the power, 2017-03-21 22:38:30]

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

[commit-bot: I haz the power, 2017-03-21 22:38:32]

Dry run: CLs for remote refs other than refs/heads/master must contain
NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them

[alokp, 2017-03-21 22:45:01]

I just have a few nits. I will let jbroman/yzhen review the test.

https://codereview.chromium.org/2745293005/diff/240001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2745293005/diff/240001/BUILD.gn#newcode902
BUILD.gn:902: "//mojo/public/interfaces/bindings/tests:tests",
":tests" is redundant here. You do not need to specify target name if it matches
the directory name

https://codereview.chromium.org/2745293005/diff/240001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/gen_data_files_list.py (right):

https://codereview.chromium.org/2745293005/diff/240001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/gen_data_files_list.py:15: files =
listdir("../../mojo/public/interfaces/bindings/tests/data/validation")
can we pass the input directory as argument?

https://codereview.chromium.org/2745293005/diff/240001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/gen_data_files_list.py:17: files = [
f[:-5] for f in files if f[-5:] == '.data' ]
use endswith('.data')?

[yzshen1, 2017-03-22 17:39:33]

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

[yzshen1, 2017-03-22 17:39:34]

https://codereview.chromium.org/2745293005/diff/260001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/2745293005/diff/260001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/BUILD.gn:18:
"$target_gen_dir/data_files.txt",
In addition to generating this data_files.txt, does it make sense to copy all
the validation test cases to gen/, and set explicit data deps GN rule on all
those files?

According to the discussion on https://codereview.chromium.org/2759803002, my
understanding is that people would like to only allow external data dependencies
in gen/ (rather than random places in the source tree outside of LayoutTests/).

+CC dpranke
Hi, Dirk. What do you think? Thanks!

[damargulis, 2017-03-22 21:11:10]

On 2017/03/22 17:39:34, yzshen1 wrote:
>
https://codereview.chromium.org/2745293005/diff/260001/mojo/public/interfaces...
> File mojo/public/interfaces/bindings/tests/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2745293005/diff/260001/mojo/public/interfaces...
> mojo/public/interfaces/bindings/tests/BUILD.gn:18:
> "$target_gen_dir/data_files.txt",
> In addition to generating this data_files.txt, does it make sense to copy all
> the validation test cases to gen/, and set explicit data deps GN rule on all
> those files?
> 
> According to the discussion on https://codereview.chromium.org/2759803002, my
> understanding is that people would like to only allow external data
dependencies
> in gen/ (rather than random places in the source tree outside of
LayoutTests/).
> 
> +CC dpranke
> Hi, Dirk. What do you think? Thanks!

That makes sense to me!  I changed the python script so that it also creates a
data/ directory and copies all of the data files into it.  
Everything that is fetched is now taken from the /gen/ directory, which should
make everything a little cleaner.

[Dirk Pranke, 2017-03-23 01:00:26]

On 2017/03/22 21:11:10, damargulis wrote:
> On 2017/03/22 17:39:34, yzshen1 wrote:
> >
>
https://codereview.chromium.org/2745293005/diff/260001/mojo/public/interfaces...
> > File mojo/public/interfaces/bindings/tests/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2745293005/diff/260001/mojo/public/interfaces...
> > mojo/public/interfaces/bindings/tests/BUILD.gn:18:
> > "$target_gen_dir/data_files.txt",
> > In addition to generating this data_files.txt, does it make sense to copy
all
> > the validation test cases to gen/, and set explicit data deps GN rule on all
> > those files?
> > 
> > According to the discussion on https://codereview.chromium.org/2759803002,
my
> > understanding is that people would like to only allow external data
> dependencies
> > in gen/ (rather than random places in the source tree outside of
> LayoutTests/).
> > 
> > +CC dpranke
> > Hi, Dirk. What do you think? Thanks!
> 
> That makes sense to me!  I changed the python script so that it also creates a
> data/ directory and copies all of the data files into it.  
> Everything that is fetched is now taken from the /gen/ directory, which should
> make everything a little cleaner.

Not only should we not allow random places in the checkout, I don't want to
allow all of /gen/,
because it's big and most of it isn't needed.

Can we change things to move this into a single subdir of /gen/ ?

[damargulis, 2017-03-23 01:19:38]

On 2017/03/23 01:00:26, Dirk Pranke wrote:
> On 2017/03/22 21:11:10, damargulis wrote:
> > On 2017/03/22 17:39:34, yzshen1 wrote:
> > >
> >
>
https://codereview.chromium.org/2745293005/diff/260001/mojo/public/interfaces...
> > > File mojo/public/interfaces/bindings/tests/BUILD.gn (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2745293005/diff/260001/mojo/public/interfaces...
> > > mojo/public/interfaces/bindings/tests/BUILD.gn:18:
> > > "$target_gen_dir/data_files.txt",
> > > In addition to generating this data_files.txt, does it make sense to copy
> all
> > > the validation test cases to gen/, and set explicit data deps GN rule on
all
> > > those files?
> > > 
> > > According to the discussion on https://codereview.chromium.org/2759803002,
> my
> > > understanding is that people would like to only allow external data
> > dependencies
> > > in gen/ (rather than random places in the source tree outside of
> > LayoutTests/).
> > > 
> > > +CC dpranke
> > > Hi, Dirk. What do you think? Thanks!
> > 
> > That makes sense to me!  I changed the python script so that it also creates
a
> > data/ directory and copies all of the data files into it.  
> > Everything that is fetched is now taken from the /gen/ directory, which
should
> > make everything a little cleaner.
> 
> Not only should we not allow random places in the checkout, I don't want to
> allow all of /gen/,
> because it's big and most of it isn't needed.
> 
> Can we change things to move this into a single subdir of /gen/ ?

Sorry I should have been more precise.  Right now, the data files list exists in
/gen/mojo/public/interfaces/bindings/tests/ and the copied data files are placed
in /gen/mojo/public/interfaces/bindings/tests/data/.  
Is that a good spot or is there somewhere else that would be more appropriate?

[yzshen1, 2017-03-23 17:46:20]

A few more comments.

https://codereview.chromium.org/2745293005/diff/280001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/2745293005/diff/280001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/BUILD.gn:18:
"$target_gen_dir/data_files.txt",
please use a more specific name such as ".../data/validation_files.txt"

https://codereview.chromium.org/2745293005/diff/280001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/BUILD.gn:19: "$target_gen_dir/data",
I think outputs should list all the files instead of a directory.

That probably means you need to have two rules: one to generate the
data_files.txt (which should include both .data and .expected files); the other
to copy all the files in data_files.txt, and list all the copied files as
outputs.

GN's "read_file" and "copy" are useful. Please see "gn help read_file" and "gn
help copy".

https://codereview.chromium.org/2745293005/diff/280001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/gen_data_files_list.py (right):

https://codereview.chromium.org/2745293005/diff/280001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/gen_data_files_list.py:6: This script
finds all of the files in the resources/data/validaiton folder
A few ideas:
- This script could be a generic tool which lists all files (or use a filter
pattern to selectively list files) in a given folder. And it could live in
mojo/public/tools/bindings/

- There is no need to copy files in this script. That could be done using "copy"
GN rule. 

- Please use OptionParser instead of directly use sys.argv.

- Please use the following pattern:

define main():
  ...

if __name__ == '__main__':
  sys.exit(main())

https://codereview.chromium.org/2745293005/diff/280001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/gen_data_files_list.py:16: files = [
f[:-5] for f in listdir(sys.argv[1]) if f.endswith('.data') ]
Does listdir() return absolute path or relative path? Please make sense those
path are relative. Because it is possible that when the tests are run on bots,
the output folder (or part of it) is copied to another machine with a different
path.

https://codereview.chromium.org/2745293005/diff/280001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/mojo/validation.html (right):

https://codereview.chromium.org/2745293005/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/mojo/validation.html:11:
define(["mojo/public/interfaces/bindings/tests/validation_test_interfaces.mojom",
80 chars, please. (here and below)

https://codereview.chromium.org/2745293005/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/mojo/validation.html:17:
"mojo/public/js/validator",
Please arrange them alphabetically.

[damargulis, 2017-03-24 19:44:48]

https://codereview.chromium.org/2745293005/diff/280001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/2745293005/diff/280001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/BUILD.gn:19: "$target_gen_dir/data",
On 2017/03/23 17:46:20, yzshen1 wrote:
> I think outputs should list all the files instead of a directory.
> 
> That probably means you need to have two rules: one to generate the
> data_files.txt (which should include both .data and .expected files); the
other
> to copy all the files in data_files.txt, and list all the copied files as
> outputs.
> 
> GN's "read_file" and "copy" are useful. Please see "gn help read_file" and "gn
> help copy".
> 

I've run into some problems using the "read_file" and "copy" commands for this
pattern.  The copy action ensures that the source files exist at gn gen time,
otherwise it throws an error and aborts.  This means that if I list the sources
by using `sources = read_file("$target_gen_dir/validation_files.txt")`, that
validation_files.txt file needs to already exist.  Listing the script action as
a dependency doesn't help, because the file still won't be created until ninja
is run, so it will not exist at gn gen time.  Copy will also not accept a
directory as its source, only a list of files.  I'm not sure of any other way to
get copy to work properly without erroring out if validation_files.txt hasn't
been created yet.  

I have been unable to get an entire directory copied in any way other than using
the python script.  The main problem I see with copying in the python script
instead of using the copy action is that ninja will not know to rerun the script
if a file is added or removed from the data/validation directory.  I thought
this would be solved by adding `data = [ "data/validation/" ]`, but it does not
seem to be doing the trick.  

Do you have any ideas on how to get copy to work properly, or to get the action
command to rerun on any changes in the validation/ directory, or maybe just a
different strategy altogether that may work better?

https://codereview.chromium.org/2745293005/diff/280001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/gen_data_files_list.py (right):

https://codereview.chromium.org/2745293005/diff/280001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/gen_data_files_list.py:6: This script
finds all of the files in the resources/data/validaiton folder
On 2017/03/23 17:46:20, yzshen1 wrote:
> A few ideas:
> - This script could be a generic tool which lists all files (or use a filter
> pattern to selectively list files) in a given folder. And it could live in
> mojo/public/tools/bindings/
> 
> - There is no need to copy files in this script. That could be done using
"copy"
> GN rule. 
> 
> - Please use OptionParser instead of directly use sys.argv.
> 
> - Please use the following pattern:
> 
> define main():
>   ...
> 
> if __name__ == '__main__':
>   sys.exit(main())

I moved this file into mojo/public/tools/bindings, but since I was unable to get
the copy gn action to work for this task (as explained in the other comment), I
am still copying all of the files in this script.  If we can't get copy to work
and use this script to copy the files, does it still make sense to use this as a
tool, or does it become too specific of a task?  Additionally, I could create
two python actions, one to create the list and another to copy all of the files
in that list.  Both of those scripts could then live in the tools/ directory.

https://codereview.chromium.org/2745293005/diff/280001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/gen_data_files_list.py:16: files = [
f[:-5] for f in listdir(sys.argv[1]) if f.endswith('.data') ]
On 2017/03/23 17:46:20, yzshen1 wrote:
> Does listdir() return absolute path or relative path? Please make sense those
> path are relative. Because it is possible that when the tests are run on bots,
> the output folder (or part of it) is copied to another machine with a
different
> path.

listdir() takes in a relative path, and returns only the file names and
extensions.  The output validation_files.txt only lists the file names and
extensions.  In the tests, the directory is specified using /gen/ which gets
converted through your patch to point to the correct generated directory.

[damargulis, 2017-03-24 23:07:17]

https://codereview.chromium.org/2745293005/diff/320001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/2745293005/diff/320001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/BUILD.gn:28: sources = [
I take back all my previous comments!  Copy does accept a directory as a source,
and properly tracks the directory for changes so that any changes to a file, or
addition or deletion of a file, will trigger a rebuild.  Using this action I am
able to copy the whole directory over into /gen/ where it can be accessed by the
LayoutTest

[damargulis, 2017-03-25 00:15:54]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-25 00:16:33]

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

[commit-bot: I haz the power, 2017-03-25 00:16:36]

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

[commit-bot: I haz the power, 2017-03-25 00:16:37]

Dry run: CLs for remote refs other than refs/heads/master must contain
NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them

[damargulis, 2017-03-25 00:16:40]

The CQ bit was unchecked by damargulis@chromium.org

[yzshen1, 2017-03-27 00:29:17]

LGTM

Thanks! This is a nice change. :)

https://codereview.chromium.org/2745293005/diff/360001/mojo/public/tools/bind...
File mojo/public/tools/bindings/gen_data_files_list.py (right):

https://codereview.chromium.org/2745293005/diff/360001/mojo/public/tools/bind...
mojo/public/tools/bindings/gen_data_files_list.py:4: """Generates a list of all
files in a directory
style nit: trailing ".", please.

https://codereview.chromium.org/2745293005/diff/360001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/mojo/validation.html (right):

https://codereview.chromium.org/2745293005/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/mojo/validation.html:152: let genDirectory =
"/gen/mojo/public/interfaces/bindings/tests/";
style nit: please use single quotes in JavaScript.
Please see:
https://google.github.io/styleguide/jsguide.html#features-strings-use-single-...

(But I think line 2-5 should stay unchanged because those are not in JavaScript
and usually use double quotes.)

[alokp, 2017-03-27 01:50:09]

lgtm

https://codereview.chromium.org/2745293005/diff/360001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/2745293005/diff/360001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/BUILD.gn:11: ":validation_test_data",
nit: Would it make sense to create a single target - "test_data" - that does
both - generate data file list and copy those files?

https://codereview.chromium.org/2745293005/diff/360001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/mojo/validation.html (right):

https://codereview.chromium.org/2745293005/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/mojo/validation.html:11:
define(["mojo/public/js/bindings",
nit: formatting?

[damargulis, 2017-03-27 17:22:50]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 17:23:29]

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

[commit-bot: I haz the power, 2017-03-27 17:23:31]

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

[commit-bot: I haz the power, 2017-03-27 17:23:32]

Dry run: CLs for remote refs other than refs/heads/master must contain
NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them

[damargulis, 2017-03-27 17:29:40]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 17:30:13]

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

[commit-bot: I haz the power, 2017-03-27 18:30:59]

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

[commit-bot: I haz the power, 2017-03-27 18:31:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[damargulis, 2017-03-27 18:40:51]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[damargulis, 2017-03-27 18:41:12]

The CQ bit was unchecked by damargulis@chromium.org

[jbroman, 2017-03-27 19:01:02]

lgtm once dpranke is happy

https://codereview.chromium.org/2745293005/diff/400001/mojo/public/tools/bind...
File mojo/public/tools/bindings/gen_data_files_list.py (right):

https://codereview.chromium.org/2745293005/diff/400001/mojo/public/tools/bind...
mojo/public/tools/bindings/gen_data_files_list.py:32: with out:
super-nit: might as well combine this with the previous line?

with file(options.output, 'w') as out:

[damargulis, 2017-03-27 20:50:25]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 20:50:53]

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

[commit-bot: I haz the power, 2017-03-28 00:27:28]

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

[commit-bot: I haz the power, 2017-03-28 00:27:29]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[damargulis, 2017-03-28 00:35:19]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 00:35:57]

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

[commit-bot: I haz the power, 2017-03-28 04:18:19]

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

[commit-bot: I haz the power, 2017-03-28 04:18:20]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Dirk Pranke, 2017-04-01 01:30:56]

apologies for the delay in getting back to this ...

https://codereview.chromium.org/2745293005/diff/440001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/2745293005/diff/440001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/BUILD.gn:43:
"$target_gen_dir/data/{{source_file_part}}",
Can these be in a common directory under root_build_dir, rather than in
target_gen_dir and hence scattered all over the gen/ subtree?

[yzshen1, 2017-04-01 03:42:12]

On Mar 31, 2017 6:30 PM, <dpranke@chromium.org> wrote:

apologies for the delay in getting back to this ...


https://codereview.chromium.org/2745293005/diff/440001/
mojo/public/interfaces/bindings/tests/BUILD.gn
File mojo/public/interfaces/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/2745293005/diff/440001/
mojo/public/interfaces/bindings/tests/BUILD.gn#newcode43
mojo/public/interfaces/bindings/tests/BUILD.gn:43:
"$target_gen_dir/data/{{source_file_part}}",
Can these be in a common directory under root_build_dir, rather than in
target_gen_dir and hence scattered all over the gen/ subtree?


I have changed the zip_build script to include files in
gen/layout_test_data/. Please copy the file into it. I am working on moving
mojom.js files into it, too.


https://codereview.chromium.org/2745293005/

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[yzshen1, 2017-04-01 03:42:13]

On Mar 31, 2017 6:30 PM, <dpranke@chromium.org> wrote:

apologies for the delay in getting back to this ...


https://codereview.chromium.org/2745293005/diff/440001/
mojo/public/interfaces/bindings/tests/BUILD.gn
File mojo/public/interfaces/bindings/tests/BUILD.gn (right):

https://codereview.chromium.org/2745293005/diff/440001/
mojo/public/interfaces/bindings/tests/BUILD.gn#newcode43
mojo/public/interfaces/bindings/tests/BUILD.gn:43:
"$target_gen_dir/data/{{source_file_part}}",
Can these be in a common directory under root_build_dir, rather than in
target_gen_dir and hence scattered all over the gen/ subtree?


I have changed the zip_build script to include files in
gen/layout_test_data/. Please copy the file into it. I am working on moving
mojom.js files into it, too.


https://codereview.chromium.org/2745293005/

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

[alokp, 2017-04-01 04:33:09]

> I have changed the zip_build script to include files in
> gen/layout_test_data/. Please copy the file into it. I am working on moving
> mojom.js files into it, too.

Would we be moving all generated mojo.js files into layout_test_data/ directory
or only the ones needed for layout tests? I am not sure if this is necessary if
data deps is correct?

[yzshen1, 2017-04-01 04:40:57]

On Mar 31, 2017 9:33 PM, <alokp@chromium.org> wrote:

> I have changed the zip_build script to include files in
> gen/layout_test_data/. Please copy the file into it. I am working on
moving
> mojom.js files into it, too.

Would we be moving all generated mojo.js files into layout_test_data/
directory
or only the ones needed for layout tests? I am not sure if this is
necessary if
data deps is correct?


I am going to only copy those necessary mojom.js files into
layout_test_data/. That will make missing dependencies much easier to find
out locally.


https://codereview.chromium.org/2745293005/

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[yzshen1, 2017-04-01 04:40:58]

On Mar 31, 2017 9:33 PM, <alokp@chromium.org> wrote:

> I have changed the zip_build script to include files in
> gen/layout_test_data/. Please copy the file into it. I am working on
moving
> mojom.js files into it, too.

Would we be moving all generated mojo.js files into layout_test_data/
directory
or only the ones needed for layout tests? I am not sure if this is
necessary if
data deps is correct?


I am going to only copy those necessary mojom.js files into
layout_test_data/. That will make missing dependencies much easier to find
out locally.


https://codereview.chromium.org/2745293005/

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

[alokp, 2017-04-01 04:45:52]

> I am going to only copy those necessary mojom.js files into
> layout_test_data/. That will make missing dependencies much easier to find
> out locally.

So IIUC mojom.js for only test interfaces will be copied to layout_test_data/?
But a general foo/bar.mojom would be generated into gen/foo/bar.mojom.js?
Wouldn't it be too confusing? What do you mean by "easier to find"?

Sorry if this has already been discussed.

[yzshen1, 2017-04-01 05:09:10]

On 2017/04/01 04:45:52, alokp wrote:
> > I am going to only copy those necessary mojom.js files into
> > layout_test_data/. That will make missing dependencies much easier to find
> > out locally.
> 
> So IIUC mojom.js for only test interfaces will be copied to layout_test_data/?
> But a general foo/bar.mojom would be generated into gen/foo/bar.mojom.js?
> Wouldn't it be too confusing? What do you mean by "easier to find"?
> 
> Sorry if this has already been discussed.

Let's say we have

mojom(foo) {
  sources = [ "bar/foo.mojom" ]
}

It will generated gen/bar/foo.mojom.js (no matter it is testing interfaces or
not)

If a layout test would like to access this mojom.js file, it has to include this
foo target as input of an action "copy_layout_test_mojom_files" (which I am
working on). This action will figure out all the mojom.js that target foo has or
depends on, copy them into gen/layout_test_data.

Eventually, layout tests will only be allowed to fetch files in
gen/layout_test_data/, instead of the entire gen/ folder.

The benefit is, if a user forgets to copy files into gen/layout_test_data/, the
tests will fail immediately and locally. (Today, if a dep rule is missing,
usually the tests fail after the CL lands. I think you have encountered such an
issue recently. :))
It also makes zip_build easier. It only needs to blindly copy the entire
gen/layout_test_data/. (Today, zip_build scans a list of gen/ subdirs and
includes all mojom.js files from them. If a user needs a mojom.js file in a new
folder, he has to update the zip_build.py script in a different repo!)

[Dirk Pranke, 2017-04-02 21:39:02]

One additional thing is that this is slightly independent of data_deps;
data_deps would
ensure that we knew the file was needed at runtime, but the runtime wouldn't
actually
know how to get the file. By moving the files into gen/layout_test_data/, it
should make it
easier to handle loading the files than it would be if the paths were more
arbitrary.

All that said, this is still clunky and I'd like to find a better solution.

[alokp, 2017-04-03 17:23:14]

Yuzhu: Thanks for clarifying that generated JS bindings would still be generated
in the usual place.

Dirk: The distinction between data_deps and runtime location makes sense.

Sorry for more bikeshedding: This problem is not unique to layout tests. But any
test (like mojo service tests) with runtime dependency. The way we have solved
it on chromecast testbots that run tests on actual chromecast devices is to
teach the equivalent of zip_build script to include all data dependencies of a
test into the test package. FWIW we use GN variable "write_runtime_deps" to dump
the list of runtime deps of each test binary, which the test package script uses
to zip-up all required files and directories. As long as data_deps is correct,
test will pass. However there is no easy way to verify missing deps without
landing the patch. May be we can add something to the CQ to catch this?

The problem with an action like copy_layout_test_mojom_files is that we would
need to duplicate this logic for all tests with runtime dependency, which does
not seem nice. I am not too familiar with testing infrastructure so I might be
missing something. I just wanted to bring up the option of making zip_build
script consider data dependencies. Please ignore if this has already been
discussed.

[damargulis, 2017-04-03 18:15:23]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 18:16:16]

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

[commit-bot: I haz the power, 2017-04-03 19:31:42]

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

[commit-bot: I haz the power, 2017-04-03 19:31:44]

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

[damargulis, 2017-04-03 21:44:19]

I moved all of the generated files so that they now live in
/gen/layout_test_data/

This should create a more organized /gen/ directory, and should work with
yzshen's patch to still allow the data to be fetched properly.

[Dirk Pranke, 2017-04-04 01:31:26]

On 2017/04/03 17:23:14, alokp wrote:
> Yuzhu: Thanks for clarifying that generated JS bindings would still be
generated
> in the usual place.
> 
> Dirk: The distinction between data_deps and runtime location makes sense.
> 
> Sorry for more bikeshedding: This problem is not unique to layout tests. But
any
> test (like mojo service tests) with runtime dependency. The way we have solved
> it on chromecast testbots that run tests on actual chromecast devices is to
> teach the equivalent of zip_build script to include all data dependencies of a
> test into the test package. FWIW we use GN variable "write_runtime_deps" to
dump
> the list of runtime deps of each test binary, which the test package script
uses
> to zip-up all required files and directories. As long as data_deps is correct,
> test will pass. However there is no easy way to verify missing deps without
> landing the patch. May be we can add something to the CQ to catch this?
> 
> The problem with an action like copy_layout_test_mojom_files is that we would
> need to duplicate this logic for all tests with runtime dependency, which does
> not seem nice. I am not too familiar with testing infrastructure so I might be
> missing something. I just wanted to bring up the option of making zip_build
> script consider data dependencies. Please ignore if this has already been
> discussed.

Yes, zip_build should be taking things like data_deps into account. I agree we 
wouldn't want to do something like copy_layout_test_mojom_files for everything
that has runtime dependencies, but there are specific aspects to this particular
feature need that require the current step, given the current implementation.

There is a separate bug open to figure out how to make this work better.

[Dirk Pranke, 2017-04-04 01:31:51]

lgtm, also.

[yzshen1, 2017-04-10 00:32:33]

On 2017/04/04 01:31:51, Dirk Pranke wrote:
> lgtm, also.

LGTM++ in case you are waiting for my response. Thanks!

[damargulis, 2017-04-10 18:02:27]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 18:03:15]

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

[commit-bot: I haz the power, 2017-04-10 19:50:17]

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

[commit-bot: I haz the power, 2017-04-10 19:50:18]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[damargulis, 2017-04-10 22:16:21]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 22:16:47]

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

[damargulis, 2017-04-10 23:46:55]

On 2017/04/10 00:32:33, yzshen1 wrote:
> On 2017/04/04 01:31:51, Dirk Pranke wrote:
> > lgtm, also.
> 
> LGTM++ in case you are waiting for my response. Thanks!

Thanks!  I am working on debugging the tests, (having a little trouble since
they seem to be passing on my machine), but I will land this as soon as they are
resolved

[damargulis, 2017-04-11 00:10:51]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 00:11:33]

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

[commit-bot: I haz the power, 2017-04-11 04:19:54]

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

[commit-bot: I haz the power, 2017-04-11 04:19:56]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[damargulis, 2017-04-11 17:04:24]

I solved the issue on all but windows machines.  I am waiting on a windows
machine to debug this on, and I will land this cl as soon as that is resolved.

Thanks for all the help!

[damargulis, 2017-04-19 19:19:23]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 19:20:15]

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

[damargulis, 2017-04-19 21:06:43]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 21:07:23]

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

[commit-bot: I haz the power, 2017-04-19 22:11:01]

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

[commit-bot: I haz the power, 2017-04-19 22:11:03]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[damargulis, 2017-04-20 17:39:01]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 17:39:42]

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

[commit-bot: I haz the power, 2017-04-20 20:11:56]

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

[commit-bot: I haz the power, 2017-04-20 20:11:57]

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

[damargulis, 2017-04-21 23:07:27]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 23:08:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-22 01:03:56]

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

[commit-bot: I haz the power, 2017-04-22 01:03:57]

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

[damargulis, 2017-04-24 17:37:42]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 17:38:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 17:41:10]

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

[commit-bot: I haz the power, 2017-04-24 17:41:11]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[damargulis, 2017-04-24 17:49:08]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 17:49:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 19:52:13]

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

[commit-bot: I haz the power, 2017-04-24 19:52:17]

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

[damargulis, 2017-04-24 21:03:54]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 21:04:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[damargulis, 2017-04-24 22:42:10]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 22:43:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 00:43:33]

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

[commit-bot: I haz the power, 2017-04-25 00:43:35]

Dry run: This issue passed the CQ dry run.

[damargulis, 2017-04-25 00:44:43]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 00:45:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 02:50:09]

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

[commit-bot: I haz the power, 2017-04-25 02:50:11]

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

[damargulis, 2017-04-25 17:45:02]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 17:45:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 18:57:22]

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

[commit-bot: I haz the power, 2017-04-25 18:57:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[damargulis, 2017-04-25 19:02:09]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 19:03:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 19:39:10]

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

[commit-bot: I haz the power, 2017-04-25 19:39:12]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[damargulis, 2017-04-25 20:10:12]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 20:11:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[damargulis, 2017-04-25 21:42:05]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 21:42:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[damargulis, 2017-04-25 22:29:40]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 22:30:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[damargulis, 2017-04-25 23:26:01]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 23:26:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[damargulis, 2017-04-26 00:25:54]

The CQ bit was checked by damargulis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 00:26:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 02:35:31]

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

[commit-bot: I haz the power, 2017-04-26 02:35:33]

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

[alokp, 2017-05-03 04:20:34]

I have taken ownership of this patch here:
https://codereview.chromium.org/2853293003/