Add python script to make adding browser-element tests a bit easier

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

Trunk
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 639990 [details] [diff] [review]
Patch, v1
Attachment #639990 - Flags: review?(mounir)
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.
Attachment #639990 - Flags: review?(mounir) → review+
(Assignee)

Comment 3

5 years ago
> 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.
(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.
(Assignee)

Comment 5

5 years ago
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!
Attachment #640209 - Flags: review?(mounir)
(Assignee)

Updated

5 years ago
Attachment #639990 - Attachment is obsolete: true
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.
Attachment #640209 - Flags: review?(mounir) → review+
(Assignee)

Comment 7

5 years ago
> 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.
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7738b028e2fd
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
https://hg.mozilla.org/mozilla-central/rev/7738b028e2fd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.