771884 - Run the WebSocket server for xpcshell-tests to allow tests related to WS

Status: NEW

(Testing :: XPCShell Harness, enhancement, P3)

Description

[Thinker Li [:sinker]]

Attachment: WIP

xpcshell-tests does not run the WebSocket server (pywebsocket) before running testcases.  We can test websocket relative cases only with mochitest.  It is useful to enable websocket in xpcshell-tests.

Comment 1

[Gregory Szorc [:gps]]

Comment on attachment 640037 [details] [diff] [review]
WIP

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

This is a drive-by. I'm not a build peer. People with review powers may disagree with me.

::: testing/xpcshell/runxpcshelltests.py
@@ +35,5 @@
> +class Process(subprocess.Popen):
> +  """
> +  Represents our view of a subprocess.
> +  It adds a kill() method which allows it to be stopped explicitly.
> +  """

Can you use mozprocess? If not, consider adding this functionality to mozprocess and then switch to mozprocess.

@@ +79,5 @@
> +        subprocess.Popen(["taskkill", "/F", "/PID", pid]).wait()
> +    else:
> +      os.kill(self.pid, signal.SIGKILL)
> +
> +class WebSocketServer(object):

This seems like something useful outside of the xpcshell test runner. The code should live elsewhere where others can use it.

Comment 2

[Jason Duell]

(cc-ing smaug in case I'm missing some easy way to run websockets inside xpcshell)

How are you planning to actually test websockets within xpcshell?  We don't have a full browser environment in xpcshell, so we can't instantiate an nsIWebSocket without some extra work.

We could instantiate the lower-level necko classes (nsIWebSocketChannel/Listener), but we wouldn't be testing the full websockets stack, just a custom listener we roll ourselves.  I suspect that's a lot of effort for incomplete coverage.

If we're doing this bug just because mochitests aren't working on B2G, it might be better just to rely on the android XUL tests providing basic e10s websocket coverage, plus manually make sure 

  http://websocketstest.com/

works on B2G, and wait for B2G mochitests to get full test coverage.

Comment 3

[Thinker Li [:sinker]]

(In reply to Jason Duell (:jduell) from comment #2)
> (cc-ing smaug in case I'm missing some easy way to run websockets inside
> xpcshell)
> 
> How are you planning to actually test websockets within xpcshell?  We don't
> have a full browser environment in xpcshell, so we can't instantiate an
> nsIWebSocket without some extra work.
> 
> We could instantiate the lower-level necko classes
> (nsIWebSocketChannel/Listener), but we wouldn't be testing the full
> websockets stack, just a custom listener we roll ourselves.  I suspect
> that's a lot of effort for incomplete coverage.

I am working on push notificaiton service.  The underlying protocol shifts to websocket recently.  As you have said, we can not use nsIWebSocket without a window, now.  So, I use nsIWebSocketChannel.  I would like to run some testcases with xpcshell to fasten my development process.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(I may have changed my mind here...) Could we make nsIWebSocket available to components?
That would be at least less error prone than using WebSocketChannel

Comment 5

[Thinker Li [:sinker]]

(In reply to Olli Pettay [:smaug] from comment #4)
> (I may have changed my mind here...) Could we make nsIWebSocket available to
> components?
> That would be at least less error prone than using WebSocketChannel

It would be better.

Comment 6

[Thinker Li [:sinker]]

Attachment: WIP v2

Refactor WebSocketServer to a module.  Use mozprocess instead of subprocess.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 651712 [details] [diff] [review]
WIP v2

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

Looking better.

::: testing/mozbase/mozprocess/mozprocess/processhandler.py
@@ +755,5 @@
>              return (f.readline(), False)
>  
> +    @property
> +    def pid(self):
> +        return self.proc.pid

Files in testing/mozbase are imported from https://github.com/mozilla/mozbase. You'll need to submit the patch via Git (ping jhammel on what the bug/commit flow looks like) then get that repository dumped/exported to mozilla-central.

::: testing/xpcshell/Makefile.in
@@ +68,5 @@
> +$(WEBSOCKET_FILES): $(topsrcdir)/testing/mochitest/pywebsocket_wrapper.py
> +	$(INSTALL) $^ $(DEPTH)/_tests/xpcshell
> +	$(INSTALL) $(topsrcdir)/testing/mochitest/pywebsocket $(DEPTH)/_tests/xpcshell
> +
> +libs:: $(WEBSOCKET_FILES)

One-off Makefile rules are bad. Ignore that they occur everywhere in this specific Makefile.in (it is a bad example).

Here, you are doing a simple file install, so you can use the generic install/copy rule from https://hg.mozilla.org/mozilla-central/file/fb2d41f41c15/config/rules.mk#l1623

It would look something like:

WEBSOCKET_FILES := pywebsocket_wrapper.py
WEBSOCKET_DEST := $(DEPTH)/_tests/xpcshell
INSTALL_TARGETS += WEBSOCKET

Although, you'll need to install a full directory there. I'm not sure if this handles directories gracefully. Try and find out!

::: testing/xpcshell/runxpcshelltests.py
@@ +44,5 @@
>      self.nodeProc = None
>  
> +  def startWebSocketServer(self, rootdir):
> +    """ Launch the websocket server """
> +    from websocketserver import WebSocketServer

This is always called. I don't see the harm in elevating this to a global import.

@@ +46,5 @@
> +  def startWebSocketServer(self, rootdir):
> +    """ Launch the websocket server """
> +    from websocketserver import WebSocketServer
> +    env = dict(self.env)
> +    env['PYTHONPATH'] = os.path.join(rootdir, 'pywebsocket')

I think more appropriate behavior would be to append to the existing value rather than blindly replacing.

::: testing/xpcshell/websocketserver.py
@@ +1,1 @@
> +from mozprocess.processhandler import ProcessHandler

Need MPL 2.0 license.

@@ +2,5 @@
> +import sys
> +import os
> +
> +class WebSocketServer(object):
> +  "Class which encapsulates the mod_pywebsocket server"

Use 4 space indent for Python.

Yes, some Python in our tree is 2 space indent, sadly. This is a new file, so it should conform to PEP-8 style conventions.

May I recommend installing flake8 (https://bitbucket.org/tarek/flake8/) and ensuring this file raises no violations.

@@ +8,5 @@
> +  def __init__(self, port, scriptdir, env, log, interactive):
> +    self.port = port
> +    self._scriptdir = scriptdir
> +    self.env = env
> +    self.log = log

Why are you reusing a log object here? IMO you should just create a new one.

@@ +19,5 @@
> +    # ignore SIGINT so the server doesn't capture a ctrl+c meant for the
> +    # debugger.
> +    #
> +    # If we're not in an interactive debugger, the wrapper causes the server to
> +    # die silently upon receiving a SIGINT.

Use """...""" docstrings for method docs.

@@ +21,5 @@
> +    #
> +    # If we're not in an interactive debugger, the wrapper causes the server to
> +    # die silently upon receiving a SIGINT.
> +    scriptPath = 'pywebsocket_wrapper.py'
> +    script = os.path.join(self._scriptdir, scriptPath)

Style nit: No need for scriptPath variable. Inline it.

@@ +28,5 @@
> +    if self.interactive:
> +        cmd += ['--interactive']
> +    cmd += ['-p', str(self.port), '-w', self._scriptdir, '-l',      \
> +           os.path.join(self._scriptdir, "websock.log"),            \
> +           '--log-level=debug', '--allow-handlers-outside-root-dir']

Style nit: I'd prefer a bunch of cmd.append() or cmd.extend([]) on multiple lines for readability.

@@ +35,5 @@
> +    self._process.run()
> +    pid = self._process.pid
> +    if pid < 0:
> +      print "Error starting websocket server."
> +      sys.exit(2)

Does ProcessHandler not do this? If not, while you are patching ProcessHandler, you may consider add this feature there.

@@ +39,5 @@
> +      sys.exit(2)
> +    self.log.info("INFO | runtests.py | Websocket server pid: %d", pid)
> +
> +  def stop(self):
> +    self._process.kill()

Consider adding a __del__ method that issues a kill. Although, if ProcessHandler does this already (possibly via subprocess) I think we'll be fine. I don't want to get in a situation where the object reference goes away and the parent is stuck in waitpid(), etc forever.

Comment 8

[Gregory Szorc [:gps]]

Also, I don't suppose we should make running of the server conditional? It seems wasteful for me to have to spin up a new process and server if we won't be using it. Perhaps its usage could be keyed off an option in the xpcshell.ini manifest?

Comment 9

[(not currently active) Ted Mielczarek]

I support this plan.

Comment 10

[Thinker Li [:sinker]]

Attachment: Run websocket server for xpcshell-tests

Add MPLv2, refactor ProcessHandler, and change the way of installation of websocket server code, ..

Comment 11

[Thinker Li [:sinker]]

(In reply to Gregory Szorc [:gps] from comment #7)
> Comment on attachment 651712 [details] [diff] [review]
> WIP v2
> 
> Review of attachment 651712 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking better.
> 
> ::: testing/mozbase/mozprocess/mozprocess/processhandler.py
> @@ +755,5 @@
> >              return (f.readline(), False)
> >  
> > +    @property
> > +    def pid(self):
> > +        return self.proc.pid
> 
> Files in testing/mozbase are imported from
> https://github.com/mozilla/mozbase. You'll need to submit the patch via Git
> (ping jhammel on what the bug/commit flow looks like) then get that
> repository dumped/exported to mozilla-central.

I have filed a pull-request for this chunk as https://github.com/mozilla/mozbase/pull/32.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 652029 [details] [diff] [review]
Run websocket server for xpcshell-tests

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

Largely nits and minor things.

One thing holding up an r+ is a test case. Please add a test to selftest.py in testing/xpcshell which verifies that the websocket server runs.

::: js/src/config/rules.mk
@@ +1644,5 @@
>  # INSTALL_TARGETS += FOO
>  define install_file_template
>  libs:: $(2)/$(notdir $(1))
>  $(2)/$(notdir $(1)): $(1) $$(call mkdir_deps,$(2))
> +	$(INSTALL) $(3) $$< $(2)

Why did you make this change?

First, changes in rules.mk must be coordinated between config/rules.mk and js/src/config/rules.mk or you will get an error when you run client.mk.

Second, the old behavior should be correct. @D is make magic for the directory of the target. Since we are in a rule for $(2)/$(notdir $(1)), @D should evaluate to $(2).

Therefore, I think it is safe to revert this change.

::: testing/xpcshell/Makefile.in
@@ +20,5 @@
> +WEBSOCKET_FILES := \
> +  $(topsrcdir)/testing/mochitest/pywebsocket_wrapper.py \
> +  $(topsrcdir)/testing/mochitest/pywebsocket \
> +  $(NULL)
> +WEBSOCKET_DEST := $(DEPTH)/_tests/xpcshell 

Nuke trailing whitespace.

Also, rules.mk defines a $(testxpcobjdir) variable. You can use that here, but you'll have to use deferred assignment (= not :=) because that variable isn't defined yet. We should consider moving its definition to config.mk or something, but that's for another bug.

::: testing/xpcshell/runxpcshelltests.py
@@ +684,5 @@
>      if shuffle:
>        random.shuffle(self.alltests)
>  
>      xunitResults = []
> +    

Kill whitespace.

@@ +723,5 @@
> +      # 'websocket = ' line for the sections of testcases required
> +      # websocket server.
> +      if (not withWebsocket) and 'websocket' in test:
> +        self.startWebSocketServer(testsRootDir)
> +        withWebsocket = True

Instead of the complex logic here, I'd just have a function "ensureRunningWebSocketServer" or something which starts the server if necessary or no-ops if it is already running. This makes the individual test logic a little easier to read, IMO.

@@ +860,5 @@
>        xunitResults.append(xunitResult)
>  
> +    if withWebsocket:
> +      self.stopWebSocketServer()
> +      pass

I would unconditionally call stopWebSocketServer and have it take the appropriate action, depending on whether a server is running.

::: testing/xpcshell/websocketserver.py
@@ +15,5 @@
> +    def __init__(self, port, scriptdir, env, log, interactive):
> +        self.port = port
> +        self._scriptdir = scriptdir
> +        self.env = env
> +        self.log = log

Please use logging.getLogger() and have the default name be __class__. If you need to share loggers with the xpcshell runner, the logger name can be a function argument.

@@ +17,5 @@
> +        self._scriptdir = scriptdir
> +        self.env = env
> +        self.log = log
> +        self.interactive = interactive
> +        pass

Remove "pass". I see you've used "pass" often in this file. All instances of it are wrong.

Pass is a no-op operation. It is typically only used as a placeholder. It is worthless to have a "pass" after other statements. It either stands by itself or shouldn't exist.

@@ +37,5 @@
> +        if self.interactive:
> +            cmd.append('--interactive')
> +            pass
> +        cmd.extend(['-p', str(self.port), '-w', self._scriptdir, '-l'])
> +        cmd.append(os.path.join(self._scriptdir, "websock.log"))

Move the '-l' down to this line so argument pairs are defined together.

@@ +38,5 @@
> +            cmd.append('--interactive')
> +            pass
> +        cmd.extend(['-p', str(self.port), '-w', self._scriptdir, '-l'])
> +        cmd.append(os.path.join(self._scriptdir, "websock.log"))
> +        cmd.extend(['--log-level=debug', '--allow-handlers-outside-root-dir'])

So, the standalone websocket server/process has its own logging facility, eh? This is unfortunate. If I were debugging xpcshell tests using the websocket server, I would /really/ this log output to be interleaved with (or at least available to) the test runner.

multiprocessing has some logging support (http://docs.python.org/library/multiprocessing.html#logging). Although, I've never used it, so I'm not sure how well it works. On top of that, I'm not sure we have Python 2.6+ deployed on all the xpcshell test hosts. Sadness, I know.

Getting the logging right might be more trouble than its worth. So, I'm inclined to let it slide.

@@ +40,5 @@
> +        cmd.extend(['-p', str(self.port), '-w', self._scriptdir, '-l'])
> +        cmd.append(os.path.join(self._scriptdir, "websock.log"))
> +        cmd.extend(['--log-level=debug', '--allow-handlers-outside-root-dir'])
> +
> +        self._process = ProcessHandler(cmd, self.log, env=self.env)

I'm pretty sure you pass in a logging.Logger instance. But mozprocess.ProcessHandler expects a file name. I'm surprised this isn't blowing up!

FWIW, mozprocess.ProcessHandler is IMO quite silly in how it handles logging. In cases where you want to use a logging.Logger, it is completely inappropriate. I almost always find myself using mozprocess.ProcessHandlerMixin so I can hook up logging properly. I think you want to do the same here.

Comment 13

[Thinker Li [:sinker]]

Comment on attachment 652029 [details] [diff] [review]
Run websocket server for xpcshell-tests

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

::: testing/xpcshell/websocketserver.py
@@ +17,5 @@
> +        self._scriptdir = scriptdir
> +        self.env = env
> +        self.log = log
> +        self.interactive = interactive
> +        pass

"pass" is a placeholder that some (a lot) editors use it as end-of-a-block.  It helps editor or lint tools for increasing or decreasing indent levels.

Comment 14

[Thinker Li [:sinker]]

Attachment: Run websocket server for xpcshell-tests, v2

selftest.py, ProcessHandlerMixin, and other minor changes.

Comment 15

[Gregory Szorc [:gps]]

(In reply to Thinker Li [:sinker] from comment #13)
> ::: testing/xpcshell/websocketserver.py
> @@ +17,5 @@
> > +        self._scriptdir = scriptdir
> > +        self.env = env
> > +        self.log = log
> > +        self.interactive = interactive
> > +        pass
> 
> "pass" is a placeholder that some (a lot) editors use it as end-of-a-block. 
> It helps editor or lint tools for increasing or decreasing indent levels.

Really?! I've never seen this used in any popular Python code. Out of curiosity, which editors and/or tools want this?

Comment 16

[Thinker Li [:sinker]]

(In reply to Gregory Szorc [:gps] from comment #15)
> (In reply to Thinker Li [:sinker] from comment #13)
> > ::: testing/xpcshell/websocketserver.py
> > @@ +17,5 @@
> > > +        self._scriptdir = scriptdir
> > > +        self.env = env
> > > +        self.log = log
> > > +        self.interactive = interactive
> > > +        pass
> > 
> > "pass" is a placeholder that some (a lot) editors use it as end-of-a-block. 
> > It helps editor or lint tools for increasing or decreasing indent levels.
> 
> Really?! I've never seen this used in any popular Python code. Out of
> curiosity, which editors and/or tools want this?

Both emacs and vim do that.  You can use a "pass" to close a block.

Comment 17

[Gregory Szorc [:gps]]

(In reply to Thinker Li [:sinker] from comment #16)
> (In reply to Gregory Szorc [:gps] from comment #15)
> > (In reply to Thinker Li [:sinker] from comment #13)
> > > ::: testing/xpcshell/websocketserver.py
> > > @@ +17,5 @@
> > > > +        self._scriptdir = scriptdir
> > > > +        self.env = env
> > > > +        self.log = log
> > > > +        self.interactive = interactive
> > > > +        pass
> > > 
> > > "pass" is a placeholder that some (a lot) editors use it as end-of-a-block. 
> > > It helps editor or lint tools for increasing or decreasing indent levels.
> > 
> > Really?! I've never seen this used in any popular Python code. Out of
> > curiosity, which editors and/or tools want this?
> 
> Both emacs and vim do that.  You can use a "pass" to close a block.

I use vim and have no trouble leaving it out. Maybe it's the Python plugin I have installed. I know Python programmers who use emacs and they don't do this.

Anyway, the style convention that overrides all others is "follow the existing style." And, no Mozilla code AFAICT uses this convention. So, please remove it from this patch.

Comment 18

[Thinker Li [:sinker]]

(In reply to Gregory Szorc [:gps] from comment #17)
> (In reply to Thinker Li [:sinker] from comment #16)
> > (In reply to Gregory Szorc [:gps] from comment #15)
> > > (In reply to Thinker Li [:sinker] from comment #13)
> > > > ::: testing/xpcshell/websocketserver.py
> > > > @@ +17,5 @@
> > > > > +        self._scriptdir = scriptdir
> > > > > +        self.env = env
> > > > > +        self.log = log
> > > > > +        self.interactive = interactive
> > > > > +        pass
> > > > 
> > > > "pass" is a placeholder that some (a lot) editors use it as end-of-a-block. 
> > > > It helps editor or lint tools for increasing or decreasing indent levels.
> > > 
> > > Really?! I've never seen this used in any popular Python code. Out of
> > > curiosity, which editors and/or tools want this?
> > 
> > Both emacs and vim do that.  You can use a "pass" to close a block.
> 
> I use vim and have no trouble leaving it out. Maybe it's the Python plugin I
> have installed. I know Python programmers who use emacs and they don't do
> this.

You can use emacs or vim without "pass", but you lost some features; like indent-region to reindnt a region of code.  It is very useful for refactoring.  For example, you move a block of code around, you usually need to re-indent it.  With "pass", you can do that without labor works by using indent-region or something alike.  If you use emacs or vim, you should try this.

> 
> Anyway, the style convention that overrides all others is "follow the
> existing style." And, no Mozilla code AFAICT uses this convention. So,
> please remove it from this patch.
ok!

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 656454 [details] [diff] [review]
Run websocket server for xpcshell-tests, v2

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

We should probably make the port number configurable. Can you add an option to the command line flags in runxpcshelltests.py?

We are almost at r+. Just minor changes now.

::: testing/mozbase/mozprocess/mozprocess/processhandler.py
@@ +755,5 @@
>              return (f.readline(), False)
>  
> +    @property
> +    def pid(self):
> +        return self.proc.pid

This should be removed from patch since this is an upstream change, no?

::: testing/xpcshell/runxpcshelltests.py
@@ +35,5 @@
>  class XPCShellTests(object):
>  
>    log = logging.getLogger()
>    oldcwd = os.getcwd()
> +  wsserver_running = False

Please move this to __init__

@@ +692,5 @@
>        random.shuffle(self.alltests)
>  
>      xunitResults = []
>  
> +    withWebsocket = False               # are there any request for websocket?

You don't need this variable any more.

@@ +868,5 @@
>  
>        xunitResults.append(xunitResult)
>  
> +    self.stopWebSocketServer()
> +    

Nit: trailing whitespace

::: testing/xpcshell/selftest.py
@@ +62,5 @@
>  
>  """ + "\n".join(testlines))
>  
> +    def prepareWebsocket(self):
> +        import shutil

Move import to top of file.

@@ +77,5 @@
> +
> +        # copy pywebsocket/
> +        pywebsocket = os.path.join(mochitest, 'pywebsocket')
> +        pywebsocket_dst = os.path.join(self.tempdir, 'pywebsocket')
> +        shutil.copytree(pywebsocket, pywebsocket_dst)

We should just install all of these bits in the virtualenv. Although, this is beyond the scope of this bug, I think.

@@ +380,5 @@
> +function run_test() {
> +  do_test_pending();
> +  let ws = createWS(make_sure_websocket_connected);
> +}
> +''')

If you are writing files with in-line static content, I'd much prefer for this content to be stored as separate files in the repository and for them to be copied in-place at run-time. Alternatively, move these blocks to file-level variables so the code reads easier.

::: testing/xpcshell/websocketserver.py
@@ +55,5 @@
> +        if self.interactive:
> +            cmd.append('--interactive')
> +            pass
> +        cmd.extend(['-p', str(self.port), '-w', self._scriptdir])
> +        cmd.extend(['-l', os.path.join(self._scriptdir, "websock.log")])

Does the websocket server write logs to both stdout/stderr and a log file? If so, I don't think there's any need to enable file-based logging since the output is being captured on the caller's logger.

Comment 20

[Thinker Li [:sinker]]

Attachment: Run websocket server for xpcshell-tests, v3

minor changes according comment #19

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 658389 [details] [diff] [review]
Run websocket server for xpcshell-tests, v3

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

Looks good!

Comment 22

[Thinker Li [:sinker]]

Attachment: Run websocket server for xpcshell-tests, v4

https://tbpl.mozilla.org/?tree=Try&rev=d953d03d4a55

Comment 23

[Ryan VanderMeulen [:RyanVM]]

(In reply to Thinker Li [:sinker] from comment #22)
> https://tbpl.mozilla.org/?tree=Try&rev=d953d03d4a55

This is showing failures galore on make check. Please don't request checkin unless the Try push is actually green.

TEST-UNEXPECTED-FAIL | /tmp/tmpIxrkLs/test_websocket.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | xpcshell/head.js | [Exception... "Component returned failure code: 0x80570018 (NS_ERROR_XPC_BAD_IID) [nsIJSCID.createInstance]"  nsresult: "0x80570018 (NS_ERROR_XPC_BAD_IID)"  location: "JS frame :: /tmp/tmpIxrkLs/test_websocket.js :: createWS :: line 12"  data: no]
make[1]: *** [check] Error 1

Comment 25

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.