Adds a generator for writing messages to disk

Adds a generator for writing messages to disk

The plan with this is to use the generator to create binary test
files. We'll write bindings tests that read these files and verify
everything is sane.

BUG=none
TEST=none
R=darin@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261605

[darin (slow to review), 2014-03-11 06:07:26]

Where do you think the actual tests will live? I think this standalone program
should probably live outside of bindings/lib since it is not part of that static
library. It could live next to the tests that depend on it, or it could live
under mojo/tools/.

[sky, 2014-03-11 14:52:47]

On 2014/03/11 06:07:26, darin wrote:
> Where do you think the actual tests will live? I think this standalone program
> should probably live outside of bindings/lib since it is not part of that
static
> library. It could live next to the tests that depend on it, or it could live
> under mojo/tools/.

I was thinking the tests would be with the corresponding bindings specific
tests.
I like mojo/tools though. Will move there.

[darin (slow to review), 2014-03-11 15:10:56]

I think we've had the goal to make mojo/public be as self contained as
possible. This includes tests, so probably this program and generated data
should live in public/bindings/tests?

Also, you might consider making the program output an ACSII representation
of the binary data. That way git will be happy. It doesn't deal well with
binary blobs. Perhaps the program could output the binary data as a C char
array literal?

-Darin
On Mar 11, 2014 7:52 AM, <sky@chromium.org> wrote:

> On 2014/03/11 06:07:26, darin wrote:
>
>> Where do you think the actual tests will live? I think this standalone
>> program
>> should probably live outside of bindings/lib since it is not part of that
>>
> static
>
>> library. It could live next to the tests that depend on it, or it could
>> live
>> under mojo/tools/.
>>
>
> I was thinking the tests would be with the corresponding bindings specific
> tests.
> I like mojo/tools though. Will move there.
>
> https://codereview.chromium.org/191293018/
>

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

[sky, 2014-03-11 16:26:00]

On 2014/03/11 15:10:56, darin wrote:
> I think we've had the goal to make mojo/public be as self contained as
> possible. This includes tests, so probably this program and generated data
> should live in public/bindings/tests?

Done.

> Also, you might consider making the program output an ACSII representation
> of the binary data. That way git will be happy. It doesn't deal well with
> binary blobs. Perhaps the program could output the binary data as a C char
> array literal?

I think this would make it more tedious and error prone for the test
implementations that need to read this. Also, this files are trivially small and
I hate to have to do something complex because of git.

  -Scott

> 
> -Darin
> On Mar 11, 2014 7:52 AM, <mailto:sky@chromium.org> wrote:
> 
> > On 2014/03/11 06:07:26, darin wrote:
> >
> >> Where do you think the actual tests will live? I think this standalone
> >> program
> >> should probably live outside of bindings/lib since it is not part of that
> >>
> > static
> >
> >> library. It could live next to the tests that depend on it, or it could
> >> live
> >> under mojo/tools/.
> >>
> >
> > I was thinking the tests would be with the corresponding bindings specific
> > tests.
> > I like mojo/tools though. Will move there.
> >
> > https://codereview.chromium.org/191293018/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[viettrungluu, 2014-03-11 18:18:38]

On 2014/03/11 16:26:00, sky wrote:
> On 2014/03/11 15:10:56, darin wrote:
> > I think we've had the goal to make mojo/public be as self contained as
> > possible. This includes tests, so probably this program and generated data
> > should live in public/bindings/tests?
> 
> Done.
> 
> > Also, you might consider making the program output an ACSII representation
> > of the binary data. That way git will be happy. It doesn't deal well with
> > binary blobs. Perhaps the program could output the binary data as a C char
> > array literal?
> 
> I think this would make it more tedious and error prone for the test
> implementations that need to read this. Also, this files are trivially small
and
> I hate to have to do something complex because of git.

FWIW, I don't think that a) binary files make git unhappy, or b) git would be
any (or much) happier with binary serialized to text.

(The trouble with "binary" files is that a) they don't diff very well, and b)
every git checkout has full history. Using text files might only be better if
they diff considerably better. In any case, it should never be a concern for
small files.)

> 
>   -Scott
> 
> > 
> > -Darin
> > On Mar 11, 2014 7:52 AM, <mailto:sky@chromium.org> wrote:
> > 
> > > On 2014/03/11 06:07:26, darin wrote:
> > >
> > >> Where do you think the actual tests will live? I think this standalone
> > >> program
> > >> should probably live outside of bindings/lib since it is not part of that
> > >>
> > > static
> > >
> > >> library. It could live next to the tests that depend on it, or it could
> > >> live
> > >> under mojo/tools/.
> > >>
> > >
> > > I was thinking the tests would be with the corresponding bindings specific
> > > tests.
> > > I like mojo/tools though. Will move there.
> > >
> > > https://codereview.chromium.org/191293018/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-03-11 19:09:09]

On Tue, Mar 11, 2014 at 11:18 AM, <viettrungluu@chromium.org> wrote:

> On 2014/03/11 16:26:00, sky wrote:
>
>> On 2014/03/11 15:10:56, darin wrote:
>> > I think we've had the goal to make mojo/public be as self contained as
>> > possible. This includes tests, so probably this program and generated
>> data
>> > should live in public/bindings/tests?
>>
>
>  Done.
>>
>
>  > Also, you might consider making the program output an ACSII
>> representation
>> > of the binary data. That way git will be happy. It doesn't deal well
>> with
>> > binary blobs. Perhaps the program could output the binary data as a C
>> char
>> > array literal?
>>
>
>  I think this would make it more tedious and error prone for the test
>> implementations that need to read this. Also, this files are trivially
>> small
>>
> and
>
>> I hate to have to do something complex because of git.
>>
>
> FWIW, I don't think that a) binary files make git unhappy, or b) git would
> be
> any (or much) happier with binary serialized to text.
>

I'm probably thinking of the try bots or commit queue. Or, perhaps the
issue I recall has been solved.


>
> (The trouble with "binary" files is that a) they don't diff very well, and
> b)
> every git checkout has full history. Using text files might only be better
> if
> they diff considerably better. In any case, it should never be a concern
> for
> small files.)
>
>
>
>     -Scott
>>
>
>  >
>> > -Darin
>> > On Mar 11, 2014 7:52 AM, <mailto:sky@chromium.org> wrote:
>> >
>> > > On 2014/03/11 06:07:26, darin wrote:
>> > >
>> > >> Where do you think the actual tests will live? I think this
>> standalone
>> > >> program
>> > >> should probably live outside of bindings/lib since it is not part of
>> that
>> > >>
>> > > static
>> > >
>> > >> library. It could live next to the tests that depend on it, or it
>> could
>> > >> live
>> > >> under mojo/tools/.
>> > >>
>> > >
>> > > I was thinking the tests would be with the corresponding bindings
>> specific
>> > > tests.
>> > > I like mojo/tools though. Will move there.
>> > >
>> > > https://codereview.chromium.org/191293018/
>> > >
>> >
>> > To unsubscribe from this group and stop receiving emails from it, send
>> an
>> email
>> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/191293018/
>

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

[viettrungluu, 2014-03-11 19:21:11]

On Tue, Mar 11, 2014 at 12:09 PM, Darin Fisher <darin@chromium.org> wrote:

>
>
>
> On Tue, Mar 11, 2014 at 11:18 AM, <viettrungluu@chromium.org> wrote:
>
>> On 2014/03/11 16:26:00, sky wrote:
>>
>>> On 2014/03/11 15:10:56, darin wrote:
>>> > I think we've had the goal to make mojo/public be as self contained as
>>> > possible. This includes tests, so probably this program and generated
>>> data
>>> > should live in public/bindings/tests?
>>>
>>
>>  Done.
>>>
>>
>>  > Also, you might consider making the program output an ACSII
>>> representation
>>> > of the binary data. That way git will be happy. It doesn't deal well
>>> with
>>> > binary blobs. Perhaps the program could output the binary data as a C
>>> char
>>> > array literal?
>>>
>>
>>  I think this would make it more tedious and error prone for the test
>>> implementations that need to read this. Also, this files are trivially
>>> small
>>>
>> and
>>
>>> I hate to have to do something complex because of git.
>>>
>>
>> FWIW, I don't think that a) binary files make git unhappy, or b) git
>> would be
>> any (or much) happier with binary serialized to text.
>>
>
> I'm probably thinking of the try bots or commit queue. Or, perhaps the
> issue I recall has been solved.
>

That's a good point. Last I checked, uploaded diffs don't include binary
file diffs, which would mean that neither git cl try nor the CQ work (I'm
not sure about git try).


>
>
>>
>> (The trouble with "binary" files is that a) they don't diff very well,
>> and b)
>> every git checkout has full history. Using text files might only be
>> better if
>> they diff considerably better. In any case, it should never be a concern
>> for
>> small files.)
>>
>>
>>
>>     -Scott
>>>
>>
>>  >
>>> > -Darin
>>> > On Mar 11, 2014 7:52 AM, <mailto:sky@chromium.org> wrote:
>>> >
>>> > > On 2014/03/11 06:07:26, darin wrote:
>>> > >
>>> > >> Where do you think the actual tests will live? I think this
>>> standalone
>>> > >> program
>>> > >> should probably live outside of bindings/lib since it is not part
>>> of that
>>> > >>
>>> > > static
>>> > >
>>> > >> library. It could live next to the tests that depend on it, or it
>>> could
>>> > >> live
>>> > >> under mojo/tools/.
>>> > >>
>>> > >
>>> > > I was thinking the tests would be with the corresponding bindings
>>> specific
>>> > > tests.
>>> > > I like mojo/tools though. Will move there.
>>> > >
>>> > > https://codereview.chromium.org/191293018/
>>> > >
>>> >
>>> > To unsubscribe from this group and stop receiving emails from it, send
>>> an
>>> email
>>> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>>>
>>
>>
>>
>> https://codereview.chromium.org/191293018/
>>
>
>

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

[sky, 2014-03-11 21:28:19]

I still like sticking with binary files here. Do you guys feel strongly that I
should change to some other encoding?

[viettrungluu, 2014-03-13 00:56:57]

On 2014/03/11 21:28:19, sky wrote:
> I still like sticking with binary files here. Do you guys feel strongly that I
> should change to some other encoding?

I have no particular opinion, FWIW.

Personally, I'd prefer binary files, but I don't know that it's worth fighting
our infrastructure over that.

[darin (slow to review), 2014-03-13 04:26:04]

On 2014/03/13 00:56:57, viettrungluu wrote:
> On 2014/03/11 21:28:19, sky wrote:
> > I still like sticking with binary files here. Do you guys feel strongly that
I
> > should change to some other encoding?
> 
> I have no particular opinion, FWIW.
> 
> Personally, I'd prefer binary files, but I don't know that it's worth fighting
> our infrastructure over that.

One approach to using binary files is to commit the binary file manually in a
separate CL. Then, you can have another CL that makes use of it, and that CL can
go through the try bots & commit queue.

Scott and I also discussed generating a C char array instead, which might work
just fine. He was going to investigate that option.

[sky, 2014-03-13 04:46:46]

Ya, sorry, I'll come back to this once I've landed some other patches
(mac bots are killing me!).

  -Scott

On Wed, Mar 12, 2014 at 9:26 PM,  <darin@chromium.org> wrote:
> On 2014/03/13 00:56:57, viettrungluu wrote:
>>
>> On 2014/03/11 21:28:19, sky wrote:
>> > I still like sticking with binary files here. Do you guys feel strongly
>> > that
>
> I
>>
>> > should change to some other encoding?
>
>
>> I have no particular opinion, FWIW.
>
>
>> Personally, I'd prefer binary files, but I don't know that it's worth
>> fighting
>> our infrastructure over that.
>
>
> One approach to using binary files is to commit the binary file manually in
> a
> separate CL. Then, you can have another CL that makes use of it, and that CL
> can
> go through the try bots & commit queue.
>
> Scott and I also discussed generating a C char array instead, which might
> work
> just fine. He was going to investigate that option.
>
> https://codereview.chromium.org/191293018/

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

[sky, 2014-04-01 21:41:38]

I had some free time, so I've updated this. I converted from binary file to
encoding each byte as a hex string on a newline. Extremely verbose, but these
files should be small and this should be easy to parse on each platform.

[viettrungluu, 2014-04-01 22:02:50]

https://codereview.chromium.org/191293018/diff/190001/mojo/public/bindings/te...
File mojo/public/bindings/tests/message_generator.cc (right):

https://codereview.chromium.org/191293018/diff/190001/mojo/public/bindings/te...
mojo/public/bindings/tests/message_generator.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
Three things:
- The contents of mojo/public/bindings/tests have moved.
- This isn't actually a test of the bindings itself, so it probably doesn't
belong there.
- Probably, as the base/ includes suggest, it doesn't need to live under
mojo/public at all. I'd suggest mojo/tools/.

[viettrungluu, 2014-04-01 22:11:42]

https://codereview.chromium.org/191293018/diff/190001/mojo/public/bindings/te...
File mojo/public/bindings/tests/message_generator.cc (right):

https://codereview.chromium.org/191293018/diff/190001/mojo/public/bindings/te...
mojo/public/bindings/tests/message_generator.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2014/04/01 22:02:50, viettrungluu wrote:
> Three things:
> - The contents of mojo/public/bindings/tests have moved.
> - This isn't actually a test of the bindings itself, so it probably doesn't
> belong there.
> - Probably, as the base/ includes suggest, it doesn't need to live under
> mojo/public at all. I'd suggest mojo/tools/.

On second thought, maybe it should be under public/ somewhere. I'd have to think
about where.

https://codereview.chromium.org/191293018/diff/190001/mojo/public/bindings/te...
mojo/public/bindings/tests/message_generator.cc:27: result.append("0X");
Also, I suggest just using StringAppendF.

(As a side commentary, it'd probably be helpful to have it prepend some
explanatory header, e.g., "# This file is generated by ....")

https://codereview.chromium.org/191293018/diff/190001/mojo/public/bindings/te...
mojo/public/bindings/tests/message_generator.cc:36: void
GenerateMessageDataMessage(const base::FilePath& path) {
I think this should be factored into several pieces:
- a WriteMessageToFile() (that takes a Message and a file[*]); probably this
function should go into a library
- something that actually generates the Message and filename (possibly you could
do this directly in main())
The reason is that I think that we should have many more "golden" messages, not
just the single one you have below.

[*] Probably you should avoid the base/ dependency and just use stdio.h. Maybe
it should take a FILE*.

[sky, 2014-04-01 22:39:17]

On 2014/04/01 22:11:42, viettrungluu wrote:
>
https://codereview.chromium.org/191293018/diff/190001/mojo/public/bindings/te...
> File mojo/public/bindings/tests/message_generator.cc (right):
> 
>
https://codereview.chromium.org/191293018/diff/190001/mojo/public/bindings/te...
> mojo/public/bindings/tests/message_generator.cc:1: // Copyright 2014 The
> Chromium Authors. All rights reserved.
> On 2014/04/01 22:02:50, viettrungluu wrote:
> > Three things:
> > - The contents of mojo/public/bindings/tests have moved.
> > - This isn't actually a test of the bindings itself, so it probably doesn't
> > belong there.
> > - Probably, as the base/ includes suggest, it doesn't need to live under
> > mojo/public at all. I'd suggest mojo/tools/.
> 
> On second thought, maybe it should be under public/ somewhere. I'd have to
think
> about where.

Darin suggested this location in the third comment. That said, that was before
"Pantone 7619". Any suggestions?

> 
>
https://codereview.chromium.org/191293018/diff/190001/mojo/public/bindings/te...
> mojo/public/bindings/tests/message_generator.cc:27: result.append("0X");
> Also, I suggest just using StringAppendF.
> 
> (As a side commentary, it'd probably be helpful to have it prepend some
> explanatory header, e.g., "# This file is generated by ....")
> 
>
https://codereview.chromium.org/191293018/diff/190001/mojo/public/bindings/te...
> mojo/public/bindings/tests/message_generator.cc:36: void
> GenerateMessageDataMessage(const base::FilePath& path) {
> I think this should be factored into several pieces:
> - a WriteMessageToFile() (that takes a Message and a file[*]); probably this
> function should go into a library
> - something that actually generates the Message and filename (possibly you
could
> do this directly in main())
> The reason is that I think that we should have many more "golden" messages,
not
> just the single one you have below.
> 
> [*] Probably you should avoid the base/ dependency and just use stdio.h. Maybe
> it should take a FILE*.

I refactored it a bit. I still left it in the same file. If we end up wanting to
generate a ton of messages than we can move into separate files. Also, I kept
the base dependency to make life simpler:)

[darin (slow to review), 2014-04-02 05:42:28]

A couple comments:

1- I think we should keep the expected message data next to the test files. I
know we have the convention of {module}/test/data elsewhere in Chromium, but in
an effort to make mojo/public/ more self-contained, it seems like the test data
should exist there too.

2- We have tried to avoid dependencies on src/base/ from code under
mojo/public/. We should either not use src/base/ for this tool or we should move
it out of mojo/public/. Looking at the code, it seems like it would be annoying
to not use src/base/ for this tool. That implies putting the tool under
mojo/tools/ instead. It would not be part of the "SDK" in that case, but at
least it would be available to us the maintainers of mojo/public/.

[darin (slow to review), 2014-04-02 05:42:28]

A couple comments:

1- I think we should keep the expected message data next to the test files. I
know we have the convention of {module}/test/data elsewhere in Chromium, but in
an effort to make mojo/public/ more self-contained, it seems like the test data
should exist there too.

2- We have tried to avoid dependencies on src/base/ from code under
mojo/public/. We should either not use src/base/ for this tool or we should move
it out of mojo/public/. Looking at the code, it seems like it would be annoying
to not use src/base/ for this tool. That implies putting the tool under
mojo/tools/ instead. It would not be part of the "SDK" in that case, but at
least it would be available to us the maintainers of mojo/public/.

[sky, 2014-04-02 16:02:28]

I moved the test data and tool to tools

[sky, 2014-04-02 16:44:56]

After talking with Darin moved to mojo/public/data/bindings/tests .

[sky, 2014-04-02 16:45:05]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-04-02 16:45:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/191293018/250001

[commit-bot: I haz the power, 2014-04-02 16:45:17]

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

[commit-bot: I haz the power, 2014-04-02 16:45:18]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[darin (slow to review), 2014-04-02 16:46:16]

LGTM

[sky, 2014-04-02 16:48:46]

On 2014/04/02 16:44:56, sky wrote:
> After talking with Darin moved to mojo/public/data/bindings/tests .

For clarity it's the test data I moved to mojo/public/data/bindings/tests.
Generator is still in mojo/tools.

[sky, 2014-04-02 16:48:50]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-04-02 16:49:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/191293018/250001

[commit-bot: I haz the power, 2014-04-02 18:46:50]

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

[commit-bot: I haz the power, 2014-04-02 18:46:50]

Retried try job too often on win_x64_rel for step(s) base_unittests,
chrome_elf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[sky, 2014-04-03 16:11:26]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-04-03 16:12:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/191293018/250001

[commit-bot: I haz the power, 2014-04-03 17:41:46]

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

[commit-bot: I haz the power, 2014-04-03 17:41:47]

Retried try job too often on win_x64_rel for step(s) base_unittests,
chrome_elf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[sky, 2014-04-03 18:27:51]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-04-03 18:28:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/191293018/270001

[commit-bot: I haz the power, 2014-04-03 23:12:01]

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

[commit-bot: I haz the power, 2014-04-03 23:12:02]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[viettrungluu, 2014-04-04 00:25:39]

The CQ bit was checked by viettrungluu@chromium.org

[commit-bot: I haz the power, 2014-04-04 00:26:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/191293018/270001

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

Change committed as 261605