Issue 2423853002: bluetooth: Add script to generate tests based on templates

ortuno

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

4 years, 2 months ago (2016-10-17 06:30:39 UTC) #1

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-17 06:30:55 UTC) #2

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-17 07:18:29 UTC) #3

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-10-17 07:18:32 UTC) #4

ortuno

ortuno@chromium.org changed reviewers: + jyasskin@chromium.org

4 years, 2 months ago (2016-10-17 07:36:30 UTC) #5

Jeffrey Yasskin

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/LayoutTests/bluetooth/PRESUBMIT.py File third_party/WebKit/LayoutTests/bluetooth/PRESUBMIT.py (right): https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/LayoutTests/bluetooth/PRESUBMIT.py#newcode16 third_party/WebKit/LayoutTests/bluetooth/PRESUBMIT.py:16: cmd = [input_api.python_executable, test_path] Does it work to do ...

4 years, 2 months ago (2016-10-19 01:39:24 UTC) #7

ortuno

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/LayoutTests/bluetooth/PRESUBMIT.py File third_party/WebKit/LayoutTests/bluetooth/PRESUBMIT.py (right): https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/LayoutTests/bluetooth/PRESUBMIT.py#newcode16 third_party/WebKit/LayoutTests/bluetooth/PRESUBMIT.py:16: cmd = [input_api.python_executable, test_path] On 2016/10/19 at 01:39:23, Jeffrey ...

4 years, 2 months ago (2016-10-19 04:34:04 UTC) #8

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/PRESUBMIT.py (right):

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/PRESUBMIT.py:16: cmd =
[input_api.python_executable, test_path]
On 2016/10/19 at 01:39:23, Jeffrey Yasskin wrote:
> Does it work to do this unconditionally?

It does. Done.

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/generate.py (right):

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/generate.py:23: .then(device =>
device.gatt.CALLS(
On 2016/10/19 at 01:39:23, Jeffrey Yasskin wrote:
> You're missing the '[' and ']' in this call, right?

Done.

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/generate.py:85: result =
re.search(r'CALLS\(\[(.*?)\]\)', template_file_data, re.MULTILINE
On 2016/10/19 at 01:39:23, Jeffrey Yasskin wrote:
> If we have a python auto-formatter, then its output is fine, but if not, I'd
rather break the line before re.MULTILINE.

Yeah the auto formatter changed it back to this.

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/generate.py:95: while result.group() in
new_test_file_data:
On 2016/10/19 at 01:39:24, Jeffrey Yasskin wrote:
> I'm confused why this is a loop. Do we expect the same CALLS([...]) content to
appear multiple times?

Yup. Though I realized that it's pretty annoying to have to write CALLS([...])
for every instance. I introduced a new PREVIOUS_CALL token for this.

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/generate.py:98: calls =
''.join(calls.split())
On 2016/10/19 at 01:39:23, Jeffrey Yasskin wrote:
> This removes all the whitespace, right? If so, comment that. It might make
more sense to use a regex for it though.

Added a comment. Apparently this is faster than a regex and also is more
"pythonic"

http://stackoverflow.com/questions/3739909/how-to-strip-all-whitespace-from-s...

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/generate.py:109: call_test_file_data =
call_test_file_data.replace('FUNCTION_NAME', name)
On 2016/10/19 at 01:39:24, Jeffrey Yasskin wrote:
> Please mention this token in the file comment.

Done.

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/generate.py:120: def main():
On 2016/10/19 at 01:39:23, Jeffrey Yasskin wrote:
> Could you do a "for f in glob.iglob('gen-*'): os.remove(f)" to make sure
excess files don't stick around?

I'm a bit scared about silently removing files. Do you think it's OK? fwiw: the
presubmit script tests that there are no obsolete gen-* files.

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/generate.py:124: test_file_handle =
open(generated_test.path, 'w+')
On 2016/10/19 at 01:39:24, Jeffrey Yasskin wrote:
> Probably use 'wb' for the mode; you don't want \r\n pairs, and you don't need
to read from the file. Any 'w' implies the truncate:
https://docs.python.org/2/library/functions.html#open

Done. Thanks for the link.

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/generate_test.py (right):

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/generate_test.py:13: UPDATE_TIP = ('To
update the generated tests, run:\n$ python '
On 2016/10/19 at 01:39:24, Jeffrey Yasskin wrote:
> Stylistically, put the break in the string just after the '\n'.

Done.

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/getPrimaryService/gen-service-device-disconnects-before.html
(right):

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getPrimaryService/gen-service-device-disconnects-before.html:20:
.then(requestDisconnection => requestDisconnection.writeValue(
On 2016/10/19 at 01:39:24, Jeffrey Yasskin wrote:
> Can you also arrange to generate the "disconnect before vs during" axis of
tests?

I think there might be too many differences between each variation that would
introduce too much complexity in the script.

So there are two types of disconnects: developer initiated and device initiated.

For developer initiated there are three types of tests:
  - disconnect-called-before
  - disconnect-called-during-success
  - disconnect-called-during-error

For each function variation e.g. getPrimaryService we need a different argument
and a different error message.

So I think we would need to start nesting tokens: 

DISCONNECT([
  BEFORE([
    {'call1', 'error type1', 'error message1'},
    {'call2', 'error type2', 'error message2'}
  ]),
  DURING_SUCCESS([
    {'call1', 'error type1', 'error message1'},
    {'call2', 'error type2', 'error message2'}
  ]),
  DURING_ERROR([
    {'call1', 'error type1', 'error message1'},
    {'call2', 'error type2', 'error message2'}
  ])
])

ortuno

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

4 years, 2 months ago (2016-10-19 04:39:28 UTC) #9

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-19 04:42:00 UTC) #10

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-19 10:30:16 UTC) #11

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-10-19 10:30:17 UTC) #12

Jeffrey Yasskin

lgtm https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/LayoutTests/bluetooth/generate.py File third_party/WebKit/LayoutTests/bluetooth/generate.py (right): https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/LayoutTests/bluetooth/generate.py#newcode120 third_party/WebKit/LayoutTests/bluetooth/generate.py:120: def main(): On 2016/10/19 04:34:04, ortuno wrote: > ...

4 years, 2 months ago (2016-10-19 22:33:29 UTC) #13

lgtm

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/generate.py (right):

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/generate.py:120: def main():
On 2016/10/19 04:34:04, ortuno wrote:
> On 2016/10/19 at 01:39:23, Jeffrey Yasskin wrote:
> > Could you do a "for f in glob.iglob('gen-*'): os.remove(f)" to make sure
> excess files don't stick around?
> 
> I'm a bit scared about silently removing files. Do you think it's OK? fwiw:
the
> presubmit script tests that there are no obsolete gen-* files.

I *think* it's ok, especially since these files are in version control anyway,
but you're also right to be nervous. We can rely on the presubmit if you prefer.

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/getPrimaryService/gen-service-device-disconnects-before.html
(right):

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getPrimaryService/gen-service-device-disconnects-before.html:20:
.then(requestDisconnection => requestDisconnection.writeValue(
On 2016/10/19 04:34:04, ortuno wrote:
> On 2016/10/19 at 01:39:24, Jeffrey Yasskin wrote:
> > Can you also arrange to generate the "disconnect before vs during" axis of
> tests?
> 
> I think there might be too many differences between each variation that would
> introduce too much complexity in the script.
> 
> So there are two types of disconnects: developer initiated and device
initiated.
> 
> For developer initiated there are three types of tests:
>   - disconnect-called-before
>   - disconnect-called-during-success
>   - disconnect-called-during-error
> 
> For each function variation e.g. getPrimaryService we need a different
argument
> and a different error message.
> 
> So I think we would need to start nesting tokens: 
> 
> DISCONNECT([
>   BEFORE([
>     {'call1', 'error type1', 'error message1'},
>     {'call2', 'error type2', 'error message2'}
>   ]),
>   DURING_SUCCESS([
>     {'call1', 'error type1', 'error message1'},
>     {'call2', 'error type2', 'error message2'}
>   ]),
>   DURING_ERROR([
>     {'call1', 'error type1', 'error message1'},
>     {'call2', 'error type2', 'error message2'}
>   ])
> ])

Yeah, it might be something semantically like:

for (VAR1, VAR2, VAR3) of [ ('call1', 'error type 1', message 1'), (...), ...]:
  for (VAR4, VAR5) of [ ('', 'content'), ('other content', ''):
    let FILENAME = VAR1 + VAR4

?? https://github.com/google/googletest/blob/master/googletest/scripts/pump.py
does some of this sort of thing, but can't generate multiple files.

It probably makes sense to generalize this later instead of now.

https://codereview.chromium.org/2423853002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/generate.py (right):

https://codereview.chromium.org/2423853002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/generate.py:108: new_test_file_data =
new_test_file_data.replace('PREVIOUS_CALL', 'CALLS')
https://docs.python.org/2/library/stdtypes.html#str.replace says .replace()
replaces all occurrences, not just the first.

ortuno

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

4 years, 2 months ago (2016-10-20 00:26:36 UTC) #14

ortuno

The patchset sent to the CQ was uploaded after l-g-t-m from jyasskin@chromium.org Link to the ...

4 years, 2 months ago (2016-10-20 00:26:47 UTC) #16

ortuno

Thanks! https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/LayoutTests/bluetooth/generate.py File third_party/WebKit/LayoutTests/bluetooth/generate.py (right): https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/LayoutTests/bluetooth/generate.py#newcode120 third_party/WebKit/LayoutTests/bluetooth/generate.py:120: def main(): On 2016/10/19 at 22:33:29, Jeffrey Yasskin ...

4 years, 2 months ago (2016-10-20 00:27:04 UTC) #17

Thanks!

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/generate.py (right):

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/generate.py:120: def main():
On 2016/10/19 at 22:33:29, Jeffrey Yasskin wrote:
> On 2016/10/19 04:34:04, ortuno wrote:
> > On 2016/10/19 at 01:39:23, Jeffrey Yasskin wrote:
> > > Could you do a "for f in glob.iglob('gen-*'): os.remove(f)" to make sure
> > excess files don't stick around?
> > 
> > I'm a bit scared about silently removing files. Do you think it's OK? fwiw:
the
> > presubmit script tests that there are no obsolete gen-* files.
> 
> I *think* it's ok, especially since these files are in version control anyway,
but you're also right to be nervous. We can rely on the presubmit if you prefer.


Acknowledged.

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/getPrimaryService/gen-service-device-disconnects-before.html
(right):

https://codereview.chromium.org/2423853002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getPrimaryService/gen-service-device-disconnects-before.html:20:
.then(requestDisconnection => requestDisconnection.writeValue(
On 2016/10/19 at 22:33:29, Jeffrey Yasskin wrote:
> On 2016/10/19 04:34:04, ortuno wrote:
> > On 2016/10/19 at 01:39:24, Jeffrey Yasskin wrote:
> > > Can you also arrange to generate the "disconnect before vs during" axis of
> > tests?
> > 
> > I think there might be too many differences between each variation that
would
> > introduce too much complexity in the script.
> > 
> > So there are two types of disconnects: developer initiated and device
initiated.
> > 
> > For developer initiated there are three types of tests:
> >   - disconnect-called-before
> >   - disconnect-called-during-success
> >   - disconnect-called-during-error
> > 
> > For each function variation e.g. getPrimaryService we need a different
argument
> > and a different error message.
> > 
> > So I think we would need to start nesting tokens: 
> > 
> > DISCONNECT([
> >   BEFORE([
> >     {'call1', 'error type1', 'error message1'},
> >     {'call2', 'error type2', 'error message2'}
> >   ]),
> >   DURING_SUCCESS([
> >     {'call1', 'error type1', 'error message1'},
> >     {'call2', 'error type2', 'error message2'}
> >   ]),
> >   DURING_ERROR([
> >     {'call1', 'error type1', 'error message1'},
> >     {'call2', 'error type2', 'error message2'}
> >   ])
> > ])
> 
> Yeah, it might be something semantically like:
> 
> for (VAR1, VAR2, VAR3) of [ ('call1', 'error type 1', message 1'), (...),
...]:
>   for (VAR4, VAR5) of [ ('', 'content'), ('other content', ''):
>     let FILENAME = VAR1 + VAR4
> 
> 
> ?? https://github.com/google/googletest/blob/master/googletest/scripts/pump.py
does some of this sort of thing, but can't generate multiple files.
> 
> 
> It probably makes sense to generalize this later instead of now.

Acknowledged.

https://codereview.chromium.org/2423853002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/generate.py (right):

https://codereview.chromium.org/2423853002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/generate.py:108: new_test_file_data =
new_test_file_data.replace('PREVIOUS_CALL', 'CALLS')
On 2016/10/19 at 22:33:29, Jeffrey Yasskin wrote:
> https://docs.python.org/2/library/stdtypes.html#str.replace says .replace()
replaces all occurrences, not just the first.

Ah right. I got used to using the loop because in the fuzzer we had nested
tokens so a single pass to replace all occurrences wasn't enough. Done.

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-20 00:27:09 UTC) #18

commit-bot: I haz the power

Description was changed from ========== bluetooth: Add script to generate tests based on templates The ...

4 years, 2 months ago (2016-10-21 13:14:27 UTC) #20

commit-bot: I haz the power

4 years, 2 months ago (2016-10-21 13:14:28 UTC) #21

Message was sent while issue was closed.

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

Issue 2423853002: bluetooth: Add script to generate tests based on templates (Closed)

Description

Patch Set 1 #

Patch Set 2 : Clean up #

Patch Set 3 : Address jyasskin's comments #

Patch Set 4 : Remove replace loop #

Messages