test-webkitpy: push more logic into webkitpy.test.main, clean up code

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

test-webkitpy: push more logic into webkitpy.test.main, clean up code

[Dirk Pranke]

Created attachment 121965 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 121965 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121965&action=review

I like the idea.  I need to revew this when I'm less tired, sorry. :(

> Tools/Scripts/test-webkitpy:36
> +# NOTE: We intentionally limit imports from webkitpy here to minimize the
> +# chances of breaking test-webkitpy itself.

This isn't nearly as important these days now that we're not trying to support super-old versions of python, but still probably not a bad idea.

> Tools/Scripts/test-webkitpy:57
> +        import google.appengine

Does this work?  I don't think google.appengine adds itself to my python path on Mac, but I may be mistaken.

> Tools/Scripts/webkitpy/test/main.py:30
> +# NOTE: We intentionally do not depend on anything else in webkitpy here to avoid breaking test-webkitpy.

If that makes the code particularly cumbersome, I would encourage us to drop this restriction. :)  I believe it was more about not importing anything before we have a chance to clean stale pyc files.

[Adam Barth]

Comment on attachment 121965 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121965&action=review

I'll let Eric do the final review.  Two minor comments below.

> Tools/Scripts/webkitpy/test/main.py:39
> +        """Execute code prior to importing from webkitpy.unittests.

I would skip these function-level docstrings.  I know is the Python way, but I think we should follow WebKit's commenting guidelines.  (Obviously not a big deal, but I wanted to mention it.)

> Tools/Scripts/webkitpy/test/main.py:57
> +            sys.argv.remove(verbose_logging_flag)

It's not enough to remove it from command_args?  I'm not that big a fan of mutating globals...

[Dirk Pranke]

(In reply to comment #2)
> > Tools/Scripts/test-webkitpy:36
> > +# NOTE: We intentionally limit imports from webkitpy here to minimize the
> > +# chances of breaking test-webkitpy itself.
> 
> This isn't nearly as important these days now that we're not trying to support super-old versions of python, but still probably not a bad idea.
> 

Yeah, if it turns out to be cumbersome, I will revisit this, but it seems like a good idea until proven otherwise.

> > Tools/Scripts/test-webkitpy:57
> > +        import google.appengine
> 
> Does this work?  I don't think google.appengine adds itself to my python path on Mac, but I may be mistaken.
> 

This is a good question. It looks like QueueStatusServer/__init__.py does mess with the default sys.path, but only *after* it attempts to import something from google.appengine :). I need to reinstall the appengine sdk and confirm.

(In reply to comment #3)
> > Tools/Scripts/webkitpy/test/main.py:39
> > +        """Execute code prior to importing from webkitpy.unittests.
> 
> I would skip these function-level docstrings.  I know is the Python way, but I think we should follow WebKit's commenting guidelines.  (Obviously not a big deal, but I wanted to mention it.)
> 

I agree, I was trying to minimize the amount of stuff I was changing in one patch. I will remove this in a subsequent patch.

> > Tools/Scripts/webkitpy/test/main.py:57
> > +            sys.argv.remove(verbose_logging_flag)
> 
> It's not enough to remove it from command_args?  I'm not that big a fan of mutating globals...

I had left this part of the code alone, but it looks buggy as heck (inconsistently using either sys.argv or sys_argv). I will clean it up and post a new patch.

[Dirk Pranke]

Created attachment 122055 [details]
test QueueStatusServer, fix argv

[Eric Seidel (no email)]

Comment on attachment 122055 [details]
test QueueStatusServer, fix argv

Won't the __init__.py tree get called when we try to load this module in order to run main?

[Dirk Pranke]

(In reply to comment #6)
> (From update of attachment 122055 [details])
> Won't the __init__.py tree get called when we try to load this module in order to run main?

I'm not sure what you're asking, but webkitpy/__init__.py and webkitpy/test/__init__.py will both be loaded, yes. Is your concern is that this might interfere with the stale .pyc deletions? I think if one was to delete the __init__.py file, then test-webkitpy would pass once, delete the stale pyc files, and then fail on the next run. That might be good enough. 

Another way to get around this without having to leave configure_logging() and clean_packages() in test-webkitpy itself would be to test for the existence of those two files explicitly in test-webkitpy and bail out if they're missing.

Thoughts? Or are you thinking of something else?

[Eric Seidel (no email)]

Comment on attachment 122055 [details]
test QueueStatusServer, fix argv

I Think this is OK.  Chris's original design was a bit overambitious. :)

[Dirk Pranke]

Committed r104768: <http://trac.webkit.org/changeset/104768>