Closed Bug 881657 Opened 7 years ago Closed 7 years ago

[httpd] Run httpd.js globally and don't start/stop the server for each test

Categories

(Testing Graveyard :: Mozmill, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Whiteboard: [mozmill-2.0][ateamtrack: p=mozmill q=2013q3 m=1])

Attachments

(1 file, 2 obsolete files)

To lower the amount of failures (e.g. bug 777354) when loading content via httpd.js, we should run the server all the time when the browser is open. This will not stop us from working on the integration of mozhttpd later, but for now a simple change should make our tests faster and most likely more reliable. With this change we can most likely also fix bug 799557. 

We cannot land the patch for now, given that we have to also stop httpd.js before shutdown. But for that I will have to fix bug 865690 first. That way we can ensure to always cleanly shutdown the server.
And we will also fix the issue with the empty error message when trying to add a HTTP resource for a directory which does not exist.
Attached patch Patch v1.1 (obsolete) — Splinter Review
Please keep in mind that this patch depends on bug 865690, so it cannot be directly applied to master. Even if code may change in the other patch, I'm asking for review given that there is less overlap.
Attachment #760834 - Attachment is obsolete: true
Attachment #768030 - Flags: review?(ctalbert)
Comment on attachment 768030 [details] [diff] [review]
Patch v1.1

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

Have you ensured this continues to work with all the existing tests? I think it will, but I'd want to be certain. Also, I think we should protect ourselves better from running multiple httpd servers. And I want to make sure that this will run properly with the new shutdown system that you created, so let's also write a test that runs httpd during a restart test and ensure that the webserver continues to be available for the duration of the test.  I don't think we have any of those tests yet in the system.  I think that in a restart test the test methods will have to continue registering their locations with the server but that's OK. I'm mostly interested in ensuring that our one server continues to be available and that it shuts down when the application shuts down.

r-, looking forward to the more complete patch.

::: mozmill/mozmill/extension/resource/modules/frame.js
@@ +449,4 @@
>            this.runner.end();
>          }
>  
> +        httpd.stop();

Let's protect against httpd never being started here. If httpd is null then this will throw.

@@ +665,5 @@
> +  this._httpd = null;
> +};
> +
> +function startHTTPd() {
> +  httpd = new Httpd(43336);

If we're really going to enforce one httpd per session then let's enforce that. Right now, this will create a new httpd every time startHTTPd is called.
Attachment #768030 - Flags: review?(ctalbert) → review-
(In reply to Clint Talbert ( :ctalbert ) from comment #4)
> > +        httpd.stop();
> 
> Let's protect against httpd never being started here. If httpd is null then
> this will throw.

The server will be started via the Python side by calling startHTTPd(). So the instance will always be present. If it's functional or not, it will be a different thing.

> > +function startHTTPd() {
> > +  httpd = new Httpd(43336);
> 
> If we're really going to enforce one httpd per session then let's enforce
> that. Right now, this will create a new httpd every time startHTTPd is
> called.

No other code will run this method except for the comment made above. Do you think we should really enforce that? I don't see a need for it, but I can do it for sure.
(In reply to Clint Talbert ( :ctalbert ) from comment #4)
> Have you ensured this continues to work with all the existing tests? I think
> it will, but I'd want to be certain. Also, I think we should protect

Yes, it does. I have tested it with all the Mutt tests and the Mozmill tests for Firefox. 

> ourselves better from running multiple httpd servers. And I want to make
> sure that this will run properly with the new shutdown system that you
> created, so let's also write a test that runs httpd during a restart test
> and ensure that the webserver continues to be available for the duration of
> the test.  I don't think we have any of those tests yet in the system.  I

The httpd.js instance will always be available. So all of our current restart tests will cover that. At least those who make use of local test pages, like the test under tests/js/frame/httpd. That's why I haven't added any other test for it.

> think that in a restart test the test methods will have to continue
> registering their locations with the server but that's OK. I'm mostly

Yes, they have to. There is nothing we want to change here.

So please tell me what all you want to have in a more complete patch.
Flags: needinfo?(ctalbert)
Let's just fix those nits then to ensure that we always only have one httpd.js active during one run. And let's write a restart test that uses httpd.js.  R+ with those two things.
Flags: needinfo?(ctalbert)
Attached patch Patch v2Splinter Review
Rebased patch for latest changes on master, and added an additional test to test the httpd.js availability through restarts.
Attachment #768030 - Attachment is obsolete: true
Attachment #772619 - Flags: review?(andreea.matei)
Comment on attachment 772619 [details] [diff] [review]
Patch v2

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

Tests well for me.
Attachment #772619 - Flags: review?(andreea.matei) → review+
Landed as:
https://github.com/mozilla/mozmill/commit/ae72d15a7dd97672e31827a01cb4a0355acee9b0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [mozmill-2.0?] → [mozmill-2.0][ateamtrack: p=mozmill q=2013q2 m=4]
Whiteboard: [mozmill-2.0][ateamtrack: p=mozmill q=2013q2 m=4] → [mozmill-2.0][ateamtrack: p=mozmill q=2013q3 m=1]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.