719618 - Craft a no-outside-network mode for peptest

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[cmtalbert]

I should have caught this, but I didn't.  We can't run peptest inside the build infra with it hitting outside servers.  So, it needs a mode where it can self-serve all the pages it needs.  Fortunately, it has the --server-location param which will start up a webserver for us.

But the tests have no means to know what the server/port is.  Furthermore, you'd want to be able to write some kind of sane test domain in all your URLs, for instance in mochitest we use mochi.test:8888 everywhere.  This is also how mochitest can simulate different domains. You can see the list of domains it uses here[1].

Mochitest does this proxying by manipulating the proxy server settings in the profile.  I'm proposing we add this ability to mozprofile as a parametrized method that peptest could then make use of to do this same type of testing.

method: create_proxy_URL
params: 
* httpserver
** web server IP address (this will usually be what we get back from mozhttpd)
* httpport, no default, error if not specified
** web server HTTP port (this will be what we get back from mozhttpd, or from command line), no default, error if not specified
* sslport
** ssl port (this will be specified on the command line), no default, no error if None
* serverlocations
** path to a server-locations.txt file formatted along the lines of [1] so that we can control which domains we add in proxying for. No default, error if not specified.

The body of this function will add prefs into the profile we are creating just as the code in automation.py.in does[2] (lines 429 - 476).

The only thing that concerns me is that "we proxy every URL except the primary" I think that is because httpd.js is also reading server-locations.txt and is automatically providing the proxy for the primary.  So we may have to change that code in order to adapt to a mozhttpd world.  I'll need to verify this ^ obviously.

How does this sound as an interface for MozProfile?  Once we have this in mozprofile, we can write a simple patch to use this code so that we can create a non-network mode for peptest.

[1]: http://mxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt
[2]: http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#429

Comment 1

[Jeff Hammel]

See also: https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/permissions.py

Comment 2

[Phil Ringnalda (:philor)]

Why would you want to have an outside-network mode at all? The very first time I looked at it, the very first test I looked at was fetching mozilla.org and getting the element with id="header". There is such a beast right now, but there wasn't then, because the SOPA page had a <header> but no <div id="header">. Next week? Who knows?

Comment 3

[Jeff Hammel]

What we really need, IMHO, is a good (and universal) way of fetching pages and their external resources in a way consumable by test frameworks.  This, plus the proxy server that Clint mentions, allow us to run tests through the web identically (almost) too our own sandbox.

The inevitable workflow:

- a programmer is asked to create a test framework to test XYZ
- the programmer does so and seeks out resources on the web to test XYZ
- the harness is useful and makes it to production
- probably, because there is no defined path, the programmer has failed to fetch external pages and all of their resources :( It looks like we need a tool!
- eventually programmers fetch the external pages and package them up
- a new collaborator comes along and says, "Hey, I want to run these tests and work on this framework!  How do I do that?" We say "Well, you can't because we're hard-coded to use a proxy server and these internal resources that we can't distribute because of IP legal violations.  Thanks for the help, but sorry :("
- QA runs the XYZ tests but goes ":gasp!: what about *this* page on the internet? How do we make it hit this page?!?" We say, "Well we're hard-coded to use a proxy server and... please file a bug and in *no more* than two weeks, we can manually mirror this page and all of its resources so that you can test it"

Okay, its a bit of a rant for this particular bug, but this issue is neither new or introduced in peptest.  In short, while a bit OT, I think we need to have reliable and extensible solutions to this problem that make life easy for everyone.

In production we will want static resources for reliability. Always.

For development and QA testing we will want the ability to test against the web. Always.

We need a way to go from one to the other easily.

Comment 4

[William Lachance (:wlach)]

(In reply to Clint Talbert ( :ctalbert ) from comment #0)
> I should have caught this, but I didn't.  We can't run peptest inside the
> build infra with it hitting outside servers.  So, it needs a mode where it
> can self-serve all the pages it needs.  Fortunately, it has the
> --server-location param which will start up a webserver for us.
> 
> But the tests have no means to know what the server/port is.  Furthermore,
> you'd want to be able to write some kind of sane test domain in all your
> URLs, for instance in mochitest we use mochi.test:8888 everywhere.  This is
> also how mochitest can simulate different domains. You can see the list of
> domains it uses here[1].
> 
> Mochitest does this proxying by manipulating the proxy server settings in
> the profile.  I'm proposing we add this ability to mozprofile as a
> parametrized method that peptest could then make use of to do this same type
> of testing.
> 
> method: create_proxy_URL
> params: 
> * httpserver
> ** web server IP address (this will usually be what we get back from
> mozhttpd)
> * httpport, no default, error if not specified
> ** web server HTTP port (this will be what we get back from mozhttpd, or
> from command line), no default, error if not specified
> * sslport
> ** ssl port (this will be specified on the command line), no default, no
> error if None
> * serverlocations
> ** path to a server-locations.txt file formatted along the lines of [1] so
> that we can control which domains we add in proxying for. No default, error
> if not specified.
> 
> The body of this function will add prefs into the profile we are creating
> just as the code in automation.py.in does[2] (lines 429 - 476).
> 
> The only thing that concerns me is that "we proxy every URL except the
> primary" I think that is because httpd.js is also reading
> server-locations.txt and is automatically providing the proxy for the
> primary.  So we may have to change that code in order to adapt to a mozhttpd
> world.  I'll need to verify this ^ obviously.
> 
> How does this sound as an interface for MozProfile?  Once we have this in
> mozprofile, we can write a simple patch to use this code so that we can
> create a non-network mode for peptest.
> 
> [1]:
> http://mxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt
> [2]: http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#429

It actually looks like some code to do this already exists:

https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/permissions.py#L237

It appears that the right of using it is to access the permissionsmanager class that every mozprofile has, manually modify the locations variable, and then use it to write prefs to disk. You can see an example here in the mozprofile constructor:

        prefs_js, user_js = self.permission_manager.getNetworkPreferences(proxy)
        self.set_preferences(prefs_js, 'prefs.js')

This is all pretty confusing (what do proxy settings and network preferences have to do permissions?) but there you go.

Comment 5

[William Lachance (:wlach)]

(In reply to Jeff Hammel [:jhammel] from comment #3)
> What we really need, IMHO, is a good (and universal) way of fetching pages
> and their external resources in a way consumable by test frameworks.  This,
> plus the proxy server that Clint mentions, allow us to run tests through the
> web identically (almost) too our own sandbox.

wget -k -p <site> ? :)

I tend to agree with philor that it's usually best to just download a website you want to test with and use it statically. It's only a little bit of work upfront, and prevents unpleasant surprises later.

Comment 6

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #4)
> (In reply to Clint Talbert ( :ctalbert ) from comment #0)

>         prefs_js, user_js =
> self.permission_manager.getNetworkPreferences(proxy)
>         self.set_preferences(prefs_js, 'prefs.js')
> 
> This is all pretty confusing (what do proxy settings and network preferences
> have to do permissions?) but there you go.

Actually, check that, it looks like you can pass the locations you want to proxy to the mozprofile constructor and then set it to proxy by setting the proxy variable to True. So while the internals are a bit confusing, the interface is (mostly) straightforward.

Comment 7

[Mark Côté [:mcote]]

It's my understanding that the proxy solution means that all proxied domains will serve the same content.  Is it worth complicating things a bit to allow different doc roots for different domains?  For example, a test might access abc.com/index.html and def.com/index.html and expect different results, whereas this would proxy to the same page in the proposed solution (as I understand it).

Comment 8

[Mark Côté [:mcote]]

Attachment: Refactor server locations code

I refactored the server-location code out of the PermissionManager, since peptest needs to be a bit more flexible.  Specifically, peptest will require a server-locations.txt file to *not* have a primary, since the primary will be added dynamically by peptest as determined by the command-line options.  It wasn't easy to do this with the way that the PermissionsManager was originally designed.

I also added a bunch more tests.

This patch is against the mozbase code in github.

Comment 9

[Jeff Hammel]

Comment on attachment 591503 [details] [diff] [review]
Refactor server locations code

+    def __init__(self, msg=None):
+        self.msg = msg

No need to do this, Exception takes a message by default:

class Foo(Exception):
    def __str__(self):
        return str(dir(self))
raise Foo
raise Foo("hello")
=>
Traceback (most recent call last):
  File "tmp.py", line 4, in <module>
    raise Foo
__main__.Foo: ['__class__', '__delattr__', '__dict__', '__doc__', '__format__', '__getattribute__', '__getitem__', '__getslice__', '__hash__', '__init__', '__module__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__setstate__', '__sizeof__', '__str__', '__subclasshook__', '__unicode__', '__weakref__', 'args', 'message']

Use 'message'

+            else:
+                if isinstance(locations, list):

This should be elif.  There should probably also be a default 'else' case which could just raise an error instead of silently doing nothing

+                               (self._num_permissions, location.host, perm,
+                                1 if allow else 2))
Should probably document what these mean

+        privileged = filter(lambda loc: "privileged" in loc.options,
+                            self._locations)

This is actually a pretty silly use of filter and lambda.  Use a list
comprehension:

privileged = [i for i in self._locations if privileged in i]

+    def setUp(self):
+        self.profile_dir = tempfile.mkdtemp()
+        self.locations_file = tempfile.NamedTemporaryFile()
+        self.locations_file.write(self.locations)
+        self.locations_file.flush()

Its probably better just have the API accept a string, IMHO

Works for me, thanks for the refactor!

Comment 10

[Mark Côté [:mcote]]

Attachment: Refactor MozProfile server-locations code and added proxying to MozHttpd

This is a new version of the patch with changes to MozHttpd to support serving local files for proxied requests.

I also included an updated version of the original patch with jhammel's suggested changes, except for documenting the "1 if allow else 2" (I don't actually know what it means--it's from the original code) and the API accepting a string (also original code, in which a string is interpreted as a path to a server-locations file).  The lambda thing was original code too, but I changed it to a list comprehension because I agree (except the list comprehension wasn't quite right). :)

Comment 11

[Mark Côté [:mcote]]

Comment on attachment 592026 [details] [diff] [review]
Refactor MozProfile server-locations code and added proxying to MozHttpd

Flagging wlachance for review as well, since the MozHttpd changes break the existing API (used only by eideticker atm, afaik).

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 592026 [details] [diff] [review]
Refactor MozProfile server-locations code and added proxying to MozHttpd

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

I don't see anything wrong in here and I have looked at this twice now.  Keep in mind all looks Greek to me as I am unfamiliar with this code in general and it is shuffled around a lot.

Comment 13

[William Lachance (:wlach)]

Comment on attachment 592026 [details] [diff] [review]
Refactor MozProfile server-locations code and added proxying to MozHttpd

I am very happy with the mozhttpd changes. I tried them out with eideticker (which does not currently use proxies) and the net result was a nice cleanup of code due to the new request class.

I only have some very small comments about the proxy part, just coming from trying to understand what was going on:

>+class PermissionsManager(object):

I'm fine either way, but I'd personally rather call this class just plain "Permissions" (for the same reason that we call the top-level class in MozProfile 'Profile' and not 'ProfileManager')

>+    _num_permissions = 0

Why is this a global variable? Maybe there's a reason but it's not clear to me what it is. Wouldn't it make more sense to just set it to 0 inside write_permissions?

>+    def write_permission(self, locations):

First of all, this writing permissions, not just a permission. :) Second, I think 'write_database' might be a clearer name for this function since that describes more clearly what it's doing. Likewise clean_permissions might be better called clean_database.

>     def getNetworkPreferences(self, proxy=False):

Maybe rename this get_network_preferences to be consistent with the style elsewhere in mozprofile. Same thing with pacPrefs

Comment 14

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #13)
> Comment on attachment 592026 [details] [diff] [review]
> Refactor MozProfile server-locations code and added proxying to MozHttpd
> 
> I am very happy with the mozhttpd changes. I tried them out with eideticker
> (which does not currently use proxies) and the net result was a nice cleanup
> of code due to the new request class.

It occurred to me just now that the naming for some of our constructs in mozhttpd is a little weird. Specifically, we have an unnecessary "Moz" prefix on a bunch of stuff, which is totally different from what we do elsewhere in Mozbase. Maybe we should consider renaming:

MozRequest -> Request
MozRequestHandler -> RequestHandler

I'd say the same about MozHttpd itself, but we already have stuff using it. Maybe we can just leave its name in place.

Comment 15

[Mark Côté [:mcote]]

Commit 9a3af3c8534c82616f898ffc785e1eb7ba9df277 to github repo.  Will merge to m-c next week.

I made all the suggested changes in comment 13 except to _num_permissions, which is there because write_permission() (now write_db()) can be called multiple times, if more locations are added.  Granted this is probably not the best way to do it, but all the modifications to permissions.sqlite, user.js, and prefs.js need an overhaul.  Also renamed getNetworkPreferences() to just network_prefs(), since the "get" part is implied.

Comment 16

[Mark Côté [:mcote]]

Attachment: Proxy mode for peptest

This adds 3 new options:

--proxy: path to a server-locations.txt-style file, specifying urls to proxy
--server-path: path to the directory containing the proxied versions
--proxy-host-dirs: if given, when a url is proxied, the proxy looks for a directory named after the host (e.g. a test accessing http://mozilla.org/ would load <server-path>/mozilla.org/index.html)

Mochitest and such don't include a proxy-host-dirs-style option; rather, they have a single index.html containing JS that maps the requested URL to a local file.  I think the --proxy-host-dirs method is a bit cleaner and more obvious.

Comment 17

[Mark Côté [:mcote]]

I also removed test_searchGoogle.js from the manifest, at least for now, since it's going to be hard to fake.

Comment 18

[cmtalbert]

Comment on attachment 592026 [details] [diff] [review]
Refactor MozProfile server-locations code and added proxying to MozHttpd

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

I think this looks really good.  Are we attempting to proxy "whatever address the test uses" or are we going to force the set of "tests that run on tbpl" to use a known, proxied location that we can seed with the set of documents that the test uses?  This second method is far simpler and is how the mochitests work.

Mochitests know that mochi.test points at the docroot of the webserver that mochitest runs and then tests can specify a document that exists at that root, allowing access to the page via urls like: http://mochi.test/foo.html.  

Even though I realize that not everything is a mochitest, I like the idea of using mochi.test because we've trained the developers to use that and there are 100's of mochitest files that use this trick.

Comment 19

[Andrew Halberstadt [:ahal]]

Comment on attachment 594314 [details] [diff] [review]
Proxy mode for peptest

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

This looks good if the two issues are fixed. I'm not 100% sure about how the internals of mozprofile/mozhttpd now work, but I assume those were reviewed separately?  r+

::: peptest/runpeptests.py
@@ +72,5 @@
> +        enable_proxy = False
> +        locations = ServerLocations()
> +        if self.options.proxyLocations:
> +            if not self.options.serverPath:
> +                self.logger.warning('Can\' set up proxy without server path')

Missing 't'

::: tests/firefox/examples/server/mozilla.org/index.html~
@@ +1,5 @@
> +<html>
> +<body>
> +Le mozilla home page.
> +</body>
> +</html>

You probably don't want to commit this file.

Comment 20

[Mark Côté [:mcote]]

Attachment: Move firefox tests out of examples/

I was thinking that it doesn't make much sense to have the tests in an examples dir, since they'll be run by default as part of peptest. So they are examples in one sense, but they're also "real" tests, so it's confusing to have them stashed away in an examples directory.

Comment 21

[Andrew Halberstadt [:ahal]]

Comment on attachment 596705 [details] [diff] [review]
Move firefox tests out of examples/

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

Sorry, I reviewed this earlier today but I must have forgotten to submit it. Looks good. My only comment would be that if everyone else dumps their tests at the root level things may get cluttered.

Comment 22

[Mark Côté [:mcote]]

I was thinking about that... but I'm not sure what structure to use. "examples" isn't very useful/informative. :)

Comment 23

[Mark Côté [:mcote]]

Attachment: Merge from peptest github repo, Dec. 14 to Feb. 14

This is a merge of all changes to the peptest github repo from Dec. 14 to
Feb. 14. This includes the network-proxy mode.

Comment 24

[cmtalbert]

Comment on attachment 596705 [details] [diff] [review]
Move firefox tests out of examples/

This looks fine, you have the google test as simply not part of any manifest. I'd like to see another manifest generated some kind of "you can run these when you have network" manifest. Perhaps more accurately it should be called "run these from your development box, not buildbot" but otherthan that these look good.

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/121ada16c793

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/121ada16c793