Bug 33045 – Add a script to test both the Python and Perl scripts

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Jerdonek]

Now that we have test-webkit-perl, it seems appropriate to rename the executable that tests the Python code.

This idea was raised on webkit-dev:

https://lists.webkit.org/pipermail/webkit-dev/2009-December/011105.html

In a later patch we can create a "test-webkit-scripts" executable to run both the Python and Perl tests, as was discussed here:

https://lists.webkit.org/pipermail/webkit-dev/2009-December/011108.html

[Chris Jerdonek]

Created attachment 45646 [details]
Proposed patch

Setting cq- since I'm not sure that svn-apply will set the executable bit correctly when moving files:

https://bugs.webkit.org/show_bug.cgi?id=27204

[Eric Seidel (no email)]

It will not, in fact. :(  However, sounds like a perfect bug for for your skills given your perl abilities! :)

[Chris Jerdonek]

(In reply to comment #2)
> It will not, in fact. :(  However, sounds like a perfect bug for for your
> skills given your perl abilities! :)

Thanks, Eric.  I had already signed on to it. :)

[Adam Barth]

I thought the verdict from the thread was "test-webkit-scripts" ?

[Adam Barth]

Comment on attachment 45646 [details]
Proposed patch

no changelog

[Adam Barth]

Ah!  That's what I get for not reading the whole bug.  Yes, this change is fine, but we still need a ChangeLog.  :)

[Chris Jerdonek]

Thanks, Adam -- sorry about the missing ChangeLog!  I'm expanding this slightly so we can do this all in one go.  I hope that's okay.  Patch forthcoming.

[Chris Jerdonek]

Created attachment 45721 [details]
Proposed patch 2

[Adam Barth]

We seem to have lost the ability to pass --all through the wrapper script.

[Chris Jerdonek]

Created attachment 45722 [details]
Proposed patch 3

Fixed patch to have properly rooted paths and added missing bug URL.

[Chris Jerdonek]

(In reply to comment #9)
> We seem to have lost the ability to pass --all through the wrapper script.

I'll take that as an enhancement request. :) I can go ahead and add it.

[Daniel Bates]

I personally do not like the names test-webkit-scripts, test-webkit-python, and test-webkit-perl, since they are not very descriptive. 

I do agree "run-webkit-unittest" is a bit of a misnomer (at this time) because we are only running unit tests against bugzilla-tool. So, I propose, if we want to rename run-webkit-unittests, it should imply that these unit tests are against bugzilla-tool, such as run-bugzilla-tool-unittests.

Moreover, for the renaming of these scripts, I think the prefix "run" is more descriptive than "test" since it describes the script we want to execute (i.e. the name that follows run-) whereas preceding a script with "test" begs the question what are we testing? For example, consider the name test-webkit-perl. Are we testing the actual script named "webkit-perl" or testing Perl support in WebKit? or testing unit tests written in Perl? or running unit tests written in Perl?. For comparison, say we had a script named "test-support-for-CSS3-lists". Then the prefix "test" makes sense since it is obvious from the rest of the name what we are testing. That is, we are testing whether WebKit supports CSS3 lists.

By a similar argument, "test-webkit-scripts" and "test-webkit-python" are ambiguous.

[Chris Jerdonek]

Created attachment 45723 [details]
Proposed patch 4

Added support for --all flag in test-webkit-scripts, as Adam requested.  I will respond to Daniel separately.

[Chris Jerdonek]

(In reply to comment #12)

I was hoping to avoid a lengthy debate about this issue since it's so minor in the scheme of things.  But since my patch is up for review, I feel obligated to respond.  Since I'm relatively new to the project, I'm not sure how these things get decided. :)

Anyways, Daniel, I was simply suggesting that, going forward, we give new "test suite" scripts names of the form "test-<component>" instead of "run-<component>-tests".

We can discuss separately what <component> should be in each case, because that needs to be decided either way.  Perhaps giving <component> a better name in each case will ease your objections.

The naming convention I'm suggesting is that if a script begins with "test", it is a script that tests part of the WebKit code base and reports on the results.

I'm suggesting the prefix "test" instead of "run" because we currently have 15 scripts that begin with the word "run".  That prefix is used about twice as often as any other prefix we have in the Scripts folder ("update" has 8).  As things grow, it sometimes makes sense to create new categories to make things easier to find.  My feeling is that using a different prefix like "test" will make it easier to distinguish the test scripts from the others.  The resulting names are also shorter and so are easier to type.

> I personally do not like the names test-webkit-scripts, test-webkit-python, and
> test-webkit-perl, since they are not very descriptive. 
> 
> I do agree "run-webkit-unittest" is a bit of a misnomer (at this time) because
> we are only running unit tests against bugzilla-tool.

No, if you look at the code you will see that this script tests about 20 different Python modules.

> So, I propose, if we want
> to rename run-webkit-unittests, it should imply that these unit tests are
> against bugzilla-tool, such as run-bugzilla-tool-unittests.

Ditto -- that's not what the script does.

> Moreover, for the renaming of these scripts, I think the prefix "run" is more
> descriptive than "test" since it describes the script we want to execute (i.e.
> the name that follows run-)

The three test scripts in the patch don't just execute a script though.  There isn't a one-to-one correspondence.  As I said above, the Python test script tests about 20 modules.  The Perl test script tests just one Perl file, but that will change over time as we increase our code coverage of the Perl.  And test-webkit-scripts tests all of these.

Also, I'm not sure I agree with the convention of using the "run" prefix for wrapper scripts that execute another script, as you suggest.  It seems like that's an implementation issue that the end-user shouldn't have to know about.  For example, "check-webkit-style" is essentially a wrapper for modules/style.py (soon to be modules/style/checker.py), but the script is not called "run-style".  By not employing this convention, we avoid having to update the name of the wrapper script whenever the internal implementation changes, which is a good thing.  It also insulates the end-user from implementation changes.

However, it does seem reasonable to use "run" when the purpose of the script is to start some application running -- like "run-safari".  As an aside, note that "run-safari" actually executes a script called "gdb-safari" (at least in some code paths).  But we don't call it "run-gdb-safari".  I like "run-safari" more because it's a simpler name for the end user.  I'm not sure what else we use the prefix "run" for in our scripts folder.  It would be useful to have a clear heuristic, because really, every script is "running" something. :)

> whereas preceding a script with "test" begs the
> question what are we testing? For example, consider the name test-webkit-perl.
> Are we testing the actual script named "webkit-perl" or testing Perl support in
> WebKit? or testing unit tests written in Perl? or running unit tests written in
> Perl?.

This seems like more an objection to the <component> name rather than an objection to using the prefix "test".  The reason is that all of the ambiguities you raise would carry over even if we were to use the "run-webkit-perl-tests" nomenclature instead.

To address this concern, I would probably be okay with using a name like one of the following in place of test-webkit-perl--

test-webkit-perl-code
test-webkit-perl-scripts
test-perl-scripts
test-perl-code

I don't know yet which I prefer.  Thanks!

[Chris Jerdonek]

(In reply to comment #14)
> I was hoping to avoid a lengthy debate about this issue since it's so minor in
> the scheme of things.  But since my patch is up for review, I feel obligated to
> respond.  Since I'm relatively new to the project, I'm not sure how these
> things get decided. :)

Hey Daniel, I want to apologize in advance for my tone in this paragraph.  Your comments are fine and appreciated, and they're already looking to improve the patch.  I didn't mean to discourage any back and forth.  Nothing is too minor, and dialogue is what drives improvement and knowledge sharing.  I guess I wasn't looking forward to writing what I knew would be a long response on New Year's Eve.  I should have waited until tomorrow.  I hope you accept this apology.  Now off to celebrate...  Happy New Year!

[Eric Seidel (no email)]

Comment on attachment 45723 [details]
Proposed patch 4

Looks fine.

A few python comments:
1.  Should really use a class to encapsulate the logic.  Then your __main__ just calls the class Foo().main()
2.  OptionParser (from optparse) is probably easier to use than getopt.

I would have probably made:
os.path.join(scripts_directory, 'test-webkit-perl')
a function, even though you only use it twice.

def script_path(script):
  return os.path.join(scripts_directory, script)

So yeah, this python could probably be made slightly better, but looks OK over all.   Thank you for doing this!

[Chris Jerdonek]

(In reply to comment #16)
> (From update of attachment 45723 [details])
> Looks fine.
> 
> A few python comments:
> 1.  Should really use a class to encapsulate the logic.  Then your __main__
> just calls the class Foo().main()
> 2.  OptionParser (from optparse) is probably easier to use than getopt.

Thanks, Eric.  Yeah, I discovered the OptionParser class shortly after submitting that latest patch.  Neat class!  check-webkit-style uses optparse, so I didn't think to look.

Anyways, sounds good.  I'll make your suggested changes in a subsequent patch (and separately, also update check-webkit-style to use OptionParser).

Could you or someone do me a favor and commit the change -- since it sets the executable bit?  Thanks a lot.

[Eric Seidel (no email)]

We really need to fix bug 27204. :)

[Eric Seidel (no email)]

% bugzilla-tool apply-patches 33045
Cleaning working directory
Updating working directory
Fetching: https://bugs.webkit.org/show_bug.cgi?id=33045&ctype=xml
1 reviewed patch found on bug 33045.
Processing 1 patch from 1 bug.
Processing patch 45723 from bug 33045.
Failed to run "['/Projects/WebKit/WebKitTools/Scripts/svn-apply', '--reviewer', 'Eric Seidel']" exit_code: 2
cp: Scripts/run-webkit-unittests: No such file or directory
Failed to copy Scripts/run-webkit-unittests WebKitTools/Scripts/test-webkit-python. at /Projects/WebKit/WebKitTools/Scripts/svn-apply line 455, <> line 288.

Failed to run "['/Projects/WebKit/WebKitTools/Scripts/svn-apply', '--reviewer', 'Eric Seidel']" exit_code: 2

Hmm...

[Chris Jerdonek]

(In reply to comment #18)
> We really need to fix bug 27204. :)

Yeah...  If we want to fix it sooner, I can remove myself as the assignee.  I'm on a path to fix it, but in a longer-term way.  I'm refactoring it to share code between svn-apply and svn-unapply, and to be unit testable.  Since I'm not using Git yet, I can't really test the code paths without unit tests.

[Chris Jerdonek]

Created attachment 45765 [details]
Proposed patch 5

Sorry about that, Eric.  This should work.  Thanks.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45765 [details] without any errors.

[Eric Seidel (no email)]

Committed r52702: <http://trac.webkit.org/changeset/52702>