Last Comment Bug 771829 - Add python script to make adding browser-element tests a bit easier
: Add python script to make adding browser-element tests a bit easier
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-07 13:12 PDT by Justin Lebar (not reading bugmail)
Modified: 2013-04-04 13:53 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch, v1 (3.19 KB, patch)
2012-07-07 13:14 PDT, Justin Lebar (not reading bugmail)
mounir: review+
Details | Diff | Review
Patch v2 (4.21 KB, patch)
2012-07-09 07:43 PDT, Justin Lebar (not reading bugmail)
mounir: review+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2012-07-07 13:12:47 PDT
I have a script which generates browser-element tests.  It's a small thing, but it makes creating new tests feel like less of a chore.  We might as well check it in, if someone else would like to use it.
Comment 1 Justin Lebar (not reading bugmail) 2012-07-07 13:14:30 PDT
Created attachment 639990 [details] [diff] [review]
Patch, v1
Comment 2 Mounir Lamouri (:mounir) 2012-07-09 04:43:11 PDT
Comment on attachment 639990 [details] [diff] [review]
Patch, v1

Review of attachment 639990 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments addressed.

::: dom/browser-element/mochitest/createNewTest.py
@@ +9,5 @@
> +import stat
> +import argparse
> +import textwrap
> +
> +html_template = textwrap.dedent("""\

What about using string.Template()?

@@ +41,5 @@
> +
> +      var iframe = document.createElement('iframe');
> +      iframe.mozbrowser = true;
> +
> +      // XXX fill in test

Change that to:
// REMOVE ME: fill in test!

@@ +57,5 @@
> +
> +    def create_file(filename, template):
> +        path = os.path.join(os.path.dirname(sys.argv[0]), format(filename))
> +        # Create a new file, bailing with an error if the file exists.
> +        fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL)

To me, doing this would be clearer:
if (os.path.exists(path)):
  // error + fail

fd = os.open(path, "w");

@@ +61,5 @@
> +        fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL)
> +
> +        try:
> +            # This file has 777 permission when created, for whatever reason.
> +            os.fchmod(fd, stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IWGRP | stat.S_IROTH)

Can you add in the comment that you want this to be "rw-rw-r--"?

@@ +73,5 @@
> +    create_file('test_browserElement_oop_{test}.html', html_template)
> +    create_file('browserElement_{test}.js', js_template)
> +
> +    print(textwrap.dedent(format("""\
> +          Finished!  Don't forget to update the Makefile and add

Add ":" at the end.

@@ +77,5 @@
> +          Finished!  Don't forget to update the Makefile and add
> +
> +            browserElement_{test}.js
> +            test_browserElement_inproc_{test}.html
> +            test_browserElement_oop_{test}.html""")))

What about adding the trailing "\" so a copy-paste might work.

@@ +84,5 @@
> +    parser = argparse.ArgumentParser(description="Create a new browser-element testcase.")
> +    parser.add_argument('test_name')
> +    parser.add_argument('bug_number', type=int)
> +    args = parser.parse_args()
> +    main(args.test_name, args.bug_number)

That seems correct but I don't have enough time to read the doc or test: what happens if you don't pass all the arguments or pass one with an incorrect type.
Comment 3 Justin Lebar (not reading bugmail) 2012-07-09 06:45:26 PDT
> To me, doing this would be clearer:
> if (os.path.exists(path)):
>   // error + fail
>
> fd = os.open(path, "w");

That would be a race condition, of course.  I wish there were a way to pass O_EXCL without using os.open (particularly so I didn't have to chmod the file, which is bizarre), but I couldn't figure it out.

> +html_template = textwrap.dedent("""\
>
> What about using string.Template()?

That doesn't solve the dedent problem, right?  Do you think using Template would be cleaner than str.format?  I thought str.format was the Shiny New Thing.

> That seems correct but I don't have enough time to read the doc or test: what happens if you 
> don't pass all the arguments or pass one with an incorrect type.

It errors out and prints the usage.
Comment 4 Mounir Lamouri (:mounir) 2012-07-09 07:31:55 PDT
(In reply to Justin Lebar [:jlebar] from comment #3)
> > To me, doing this would be clearer:
> > if (os.path.exists(path)):
> >   // error + fail
> >
> > fd = os.open(path, "w");
> 
> That would be a race condition, of course.

You mean if the file is created after the call to os.path.exists()? I wouldn't bother about that but feel free to keep your method if you prefer to handle that use case.

> > +html_template = textwrap.dedent("""\
> >
> > What about using string.Template()?
> 
> That doesn't solve the dedent problem, right?  Do you think using Template
> would be cleaner than str.format?  I thought str.format was the Shiny New
> Thing.

Don't now. Feel free to keep str.format if you believe it's the right thing to do here. I'm no Python guru.
Comment 5 Justin Lebar (not reading bugmail) 2012-07-09 07:43:02 PDT
Created attachment 640209 [details] [diff] [review]
Patch v2

Your comment about adding backslashes to the end of the lines so they can be copy/pasted suggested to me that I could do better: Put the filenames in the Makefile.in and open $EDITOR!
Comment 6 Mounir Lamouri (:mounir) 2012-07-09 08:14:25 PDT
Comment on attachment 640209 [details] [diff] [review]
Patch v2

Review of attachment 640209 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/browser-element/mochitest/createNewTest.py
@@ +72,5 @@
> +    with open('Makefile.in', 'r') as f:
> +        num_lines = len(f.readlines())
> +
> +    subprocess.call([os.environ['EDITOR'],
> +                     '+%d' % (num_lines - len(lines_to_write) + 2),

Why +2?

@@ +73,5 @@
> +        num_lines = len(f.readlines())
> +
> +    subprocess.call([os.environ['EDITOR'],
> +                     '+%d' % (num_lines - len(lines_to_write) + 2),
> +                     'Makefile.in'])

You should probably handle the exception that is sent in case of $EDITOR points to a non-installed binary.
Comment 7 Justin Lebar (not reading bugmail) 2012-07-09 08:24:02 PDT
> Why +2?

It's not at all obvious, but suppose I have a file:

> 1  AAA
> 2  BBB
> 3
> 4  # I added the line below, but also the line above.
> 5  some_file.html

If I want to put the cursor on line 4, that's 5 - 3 + 2.

> You should probably handle the exception that is sent in case of $EDITOR points to a non-installed 
> binary.

Good call.
Comment 8 Justin Lebar (not reading bugmail) 2012-07-09 09:00:18 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/7738b028e2fd
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-07-09 18:04:08 PDT
https://hg.mozilla.org/mozilla-central/rev/7738b028e2fd

Note You need to log in before you can comment on or make changes to this bug.