Closed Bug 1262705 Opened 8 years ago Closed 8 years ago

Intermittent test_standards.py TestStandardsUnits.test_units | UnknownException: Error loading page

Categories

(Testing :: Marionette Client and Harness, defect)

defect
Not set
normal

Tracking

(firefox48 wontfix, firefox49 wontfix, firefox50 fixed, firefox51 fixed, firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: philor, Assigned: mixedpuppy)

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

The problem with this test is that we seem to hang when find_element() gets called:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/microformats/test/marionette/microformats_tester.py?q=file%3Amicroformats_tester.py&redirect_type=single#103

In this case we send the message to marionette server but never receive a reply. So we wait until the global socket timeout and die.

I can reproduce this bug locally when I simply run:

.mach marionette-test toolkit/components/microformats/test/marionette/test_standards.py

The requested page from the locally spawned webserver is not loaded. So we get a 404, which is the original exception message, but given that we hang here, i will adjust the bug summary accordingly. With bug 1202392 we will get better messages again.
Summary: Intermittent test_standards.py TestStandardsUnits.test_units | UnknownException: UnknownException: Error loading page → Intermittent test_standards.py TestStandardsUnits.test_units | TimeoutException: Process killed because the connection was lost (Reason: TimeoutException: Connection timed out) [Error loading page]
Shane, maybe you can have a look at this bug?
Flags: needinfo?(mixedpuppy)
The 404 is just the favicon. I don't see how that would cause this issue, but a simple fix is adding

link rel="icon" type="image/png" id="favicon" href="chrome://branding/content/icon32.png"

to the head section in toolkit/components/microformats/test/standards-tests/index.html

I cannot reproduce the problem, if you are could you see if that addresses the problem?
Flags: needinfo?(mixedpuppy) → needinfo?(hskupin)
The reported disconnect of the application is actually a regression from bug 1257476. I will have a fix for that in my patch on bug 1202392.

I will restore the summary to what it was before my change. So given that this failure is just for the favicon, lets get it fixed! ;)
Flags: needinfo?(hskupin)
Summary: Intermittent test_standards.py TestStandardsUnits.test_units | TimeoutException: Process killed because the connection was lost (Reason: TimeoutException: Connection timed out) [Error loading page] → Intermittent test_standards.py TestStandardsUnits.test_units | UnknownException: UnknownException: Error loading page
Ok, so the favicon is only one part which is not affecting the test run. The real issue is that you have to get the tests started always from folders matching the location of the microformat tests! It will not work when your current working directory is eg. testing:

$ cd testing
$ mach marionette-test /path/to/repo/toolkit/components/microformats/manifest.ini 

The latter should simulate a different topsrc dir as how the tests get run in packaged tests.

I don't actually have the time to get this fixed. Shane, I hope you can reproduce this issue with the above command.
Flags: needinfo?(mixedpuppy)
Ok, I can reproduce now.  I'll take a look.
Assignee: nobody → mixedpuppy
Flags: needinfo?(mixedpuppy)
Attached patch fix http root path (obsolete) — Splinter Review
This makes mach work from any directory inside a repo.  It doesn't work from outside the root repo path.
Attachment #8777164 - Flags: review?(hskupin)
Summary: Intermittent test_standards.py TestStandardsUnits.test_units | UnknownException: UnknownException: Error loading page → Intermittent test_standards.py TestStandardsUnits.test_units | UnknownException: Error loading page
Attachment #8777164 - Attachment is obsolete: true
Attachment #8777164 - Flags: review?(hskupin)
https://reviewboard.mozilla.org/r/68982/#review66330

Not sure if I'm the best person to ask for a review of those tests and customized harness script. I'm at least not a peer of toolkit so I cannot give a final review.

So I checked the changes and put some questions inline. Something I would like to see is a try build with a couple of retries of the marionette unit test job, so that we can see the issue is fixed - given that this is an intermittent test failure.

::: toolkit/components/microformats/test/marionette/microformats_tester.py:37
(Diff revision 1)
> +
>      def log_message(self, format, *args, **kwargs):
> -        pass
> +        if DEBUG:
> +            super(HttpRequestHandler, self).log_message(format, *args, **kwargs)
>  
> +    def translate_path(self, path):

I don't see that this method is used anywhere. Mind asking for what it has it been added?

::: toolkit/components/microformats/test/marionette/test_standards.py:8
(Diff revision 1)
>  import os
>  import sys
> -sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'marionette'))
> +path = os.path.normpath(os.path.join(os.path.dirname(__file__), '..', 'marionette'))
> +print "****** ",path
> +#sys.exit(0)
> +sys.path.append(os.path.normpath(os.path.join(os.path.dirname(__file__), '..', 'marionette')))

Those lines look like debug code you want to remove.
Attachment #8777436 - Flags: review?(mozilla)
Did you address Henrik's comments? I can't tell in reviewboard.
(In reply to Mike Kaply [:mkaply] from comment #12)
> Did you address Henrik's comments? I can't tell in reviewboard.

I'll remove the debugging code prior to landing.  log_message is called by the super class.  Primarily I'm overriding translate_path (copied from super class) to change one line of code that addresses this problem (ie. path = self.root).
(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> the super class.  Primarily I'm overriding translate_path (copied from super
> class) to change one line of code that addresses this problem (ie. path =
> self.root).

Why you have to copy the whole method only to change the path? Can't you simply call this method of the super class with your own custom path like:

> def translate_path(self, path):
>    super(%this_class, self).translate_path(self.root)
(In reply to Henrik Skupin (:whimboo) from comment #14)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> > the super class.  Primarily I'm overriding translate_path (copied from super
> > class) to change one line of code that addresses this problem (ie. path =
> > self.root).
> 
> Why you have to copy the whole method only to change the path? Can't you
> simply call this method of the super class with your own custom path like:

No, you cant do that.  The path argument is a path being requested via http and is translated to a local path by this function.  The super class uses the directory you execute from to calculate root (ie. the directory you run mach in).  This modified version changes root to always be relative to the location of this script.
Mind telling me which superclass the `translate_path()` method actually implements, which you overwrite here? I'm actually not able to find it. I also don't understand why we need the URL to actually retrieve the root path for the server to serve data from. We already have this path in the harness before you start the server. So why cannot we simply feed it in as root path? Maybe some links would help me to understand this problem.
The superclass is a python standard class, SimpleHTTPServer.  This test system doesn't use the Marionette http server, which could/should probably change. SimpleHTTPServer:translate_path uses os.getcwd() for the root of the path translation, which works fine if you execute from a direct parent of the test directory, but once you move to a sibling directory it fails for us.
Ah I see. So it is another path as given via the parameter to the function. Indeed, less than ideal. Here the link as reference:

https://hg.python.org/cpython/file/tip/Lib/http/server.py#l756

I think the above should help Mike to review the patch.

Shane, would you mind filing a new bug for the transition to wptserve? Maybe it could be made a mentored project, when all the requirements get clearly specified. Thanks.
Comment on attachment 8777436 [details]
Bug 1262705 fix root path for microformats test class,

https://reviewboard.mozilla.org/r/68982/#review69142
Attachment #8777436 - Flags: review?(mozilla) → review+
Shane:

Is this ready to go?
There's one small change I made after a little more testing.  New patch on review, try builds running.  Should be able to autoland once we see the try builds success.
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ca98b1602613
fix root path for microformats test class, r=mkaply
https://hg.mozilla.org/mozilla-central/rev/ca98b1602613
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Product: Testing → Remote Protocol
Moving bug to Testing::Marionette Client and Harness component per bug 1815831.
Component: Marionette → Marionette Client and Harness
Product: Remote Protocol → Testing
You need to log in before you can comment on or make changes to this bug.