1113736 - Simplify test code by replacing "tail.js" with an asynchronous termination function

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

This bug is about the automated testing for the JavaScript API for Downloads in Firefox for Desktop. This component is tested with xpcshell:

https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

At the end of each test file, some cleanup is performed:

http://dxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/test/unit/test_DownloadCore.js#91
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/test/unit/tail.js

Now that xpcshell tests support asynchronous "do_register_cleanup" calls, one should be placed in the "head.js" file, returning the Promise corresponding to the shutdown of the HTTP server.

The "tail.js" file and any references to it from the test files can then be deleted.

Comment 1

[Medhini]

Hi! Its my first time solving a bug. How do I go about it?

Comment 2

[:Paolo Amadini]

(In reply to Medhini from comment #1)
> Hi! Its my first time solving a bug. How do I go about it?

Hi! Thanks for your interest :-) This bug requires just some basic JavaScript, as well as the time to get familiar with our build and test infrastructure, and the Mercurial version control system.

The first step is getting a Firefox build, which can take some time. If you don't have one already, we have a wiki page detailing the build prerequisites and instructions:

https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

This should get you started, but let me know if you have any questions - I'll probably be around again tomorrow and next week. When the build is ready, I can give you some more details about running the specific tests and writing the JavaScript code.

Comment 3

[Medhini]

(In reply to :Paolo Amadini from comment #2)
> (In reply to Medhini from comment #1)
> > Hi! Its my first time solving a bug. How do I go about it?
> 
> Hi! Thanks for your interest :-) This bug requires just some basic
> JavaScript, as well as the time to get familiar with our build and test
> infrastructure, and the Mercurial version control system.
> 
> The first step is getting a Firefox build, which can take some time. If you
> don't have one already, we have a wiki page detailing the build
> prerequisites and instructions:
> 
> https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
> 
> This should get you started, but let me know if you have any questions -
> I'll probably be around again tomorrow and next week. When the build is
> ready, I can give you some more details about running the specific tests and
> writing the JavaScript code.

Hi! I have the Firefox build ready. How do  proceed from here?

Comment 4

[:Paolo Amadini]

(In reply to Medhini from comment #3)
> Hi! I have the Firefox build ready. How do  proceed from here?

Cool! The tests to modify are in the folder you can see from the links in comment 0. To run them all:

./mach test toolkit/components/jsdownloads/test/unit/

Now, you'll only see a summary (all test should pass). To see a detailed log for one test:

./mach test toolkit/components/jsdownloads/test/unit/test_DownloadCore.js

You can see something like "test test_common_terminate pending" near the end, but there's no clear message indicating that "tail.js" was executed. But if you add this inside "test_common_terminate":

  do_print("Stopping HTTP server.");

You should now see your message in the detailed log.

So, we need to get rid of "tail.js". You need to replace "test_common_terminate" with a "do_register_cleanup" call at the end of the "head.js" file. In doubt, you can always use DXR to search for examples of how something is used in our codebase:

http://dxr.mozilla.org/mozilla-central/search?q=do_register_cleanup

The main difference here is that the tail.js function is a "Task.jsm" task (it is a generator that can use "yield"), but the cleanup function cannot be a task, so you have to return the Promise object to the caller instead, to make the shutdown asynchronous.

When you've checked that this works as expected and tests still pass, the tail.js file can be deleted and the "//// Termination" sections can be removed from all test files.

You can then create a patch and attach it for review:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Comment 5

[Kushan Joshi [:kepta]]

Attachment: bug1113736-removing-tailjs.patch

Comment 6

[Kushan Joshi [:kepta]]

Hi,
Its been a week since Medhini gave any update. I humbly request you to allow me to work on this bug. Please review my patch.

Comment 7

[:Paolo Amadini]

(In reply to 0o3ko0 from comment #6)
> Its been a week since Medhini gave any update. I humbly request you to allow
> me to work on this bug. Please review my patch.

Hi! I really appreciate your interest, however the fact that Medhini did not update this bug for one week does not necessarily mean he isn't actively working on it, and I'd definitely be willing to wait some more, especially considering this is a holiday period in some parts of the world.

But let's ask them! Medhini, are you still working on this bug? If yes, let us know and take all the time you need - and feel free to ask any questions either on the bug or via email. If you're doing something else, still let us know, so that 0o3ko0 can take the bug.

Also, consider that you can always download, apply, and test other people's patches, sometimes it may be a good way to get familiar with our code.

Comment 8

[:Paolo Amadini]

Comment on attachment 8542104 [details] [diff] [review]
bug1113736-removing-tailjs.patch

Clearing the review flag just for now, while waiting for an answer. It's quite normal that communication plays a major role in the time required to fix a bug, so don't worry! And thanks for the patch in any case!

Comment 9

[Medhini]

Hey! Sorry about the delay. There was a problem with my build and I had to start over. I will update you about my work in the next 2 days.

Comment 10

[:Paolo Amadini]

Thanks for the update!

Comment 11

[:Paolo Amadini]

Hi Medhini, how's it going? If you're not working on this right now, I feel we should open this bug to other contributors.

Comment 12

[:Paolo Amadini]

Comment on attachment 8542104 [details] [diff] [review]
bug1113736-removing-tailjs.patch

I've opened this bug for other contributors to take. The attached patch is a good start, but has a couple issues for which it can't work yet.

We'll need a new patch with these issues addressed, I believe the comments already on the bug should help in getting started.

Comment 13

[vaibhav [:vaibhavChoudhary]]

:paolo  i would really like to work on this bug .I m new to open source and i think this can help me to further venture in open source world.

Comment 14

[:Paolo Amadini]

(In reply to vaibhav from comment #13)
> :paolo  i would really like to work on this bug .I m new to open source and
> i think this can help me to further venture in open source world.

Thanks a lot for your interest! I see you have identified a few good first bugs already. Great! Do you have an idea about which one you would like to work on first, or do you want to work with multiple patches in parallel?

For this bug, you need a Firefox build for Desktop. You should run the tests using the first command in comment 4, to see that everything is working as expected. Do you have a build already?

Comment 15

[vaibhav [:vaibhavChoudhary]]

i m setting up my development environment and it will be ready before midnight.i would greatly appreciate if u can suggest some other bugs too.

Comment 16

[anubhav.worklinux]

Hi Paolo, i would like to work on this bug if no objections here.

Comment 17

[:Paolo Amadini]

You're very welcome to take this bug!

Do you have any questions on the comments already posted here, or do you feel ready to work on a patch?

Comment 18

[Mrinal Dhar]

Attachment: Initial attempt at replacing the code in tail.js with a function call at the end of head.js

Hi, Paolo. I took this one up since there was no recent activity on this, I hope that's okay. I've read your comments on this page, and proceeded to remove the call to the function in tail.js from one of the test files. 

In the head.js file, I've added a call to do_register_cleanup at the end, although I know I didn't do it right. I also looked at the examples of its usage on the dxr page, but that didn't seem to help much. 

Could you please help me out about this? When is do_register_cleanup function called? And how does returning the promise to its caller work out?

Thanks.

Comment 19

[:Paolo Amadini]

(In reply to Mrinal Dhar from comment #18)
> Hi, Paolo. I took this one up since there was no recent activity on this, I
> hope that's okay.

Yeah, that's fine given other contributors didn't comment for a long time. Thanks!

> I've read your comments on this page, and proceeded to
> remove the call to the function in tail.js from one of the test files. 
> 
> In the head.js file, I've added a call to do_register_cleanup at the end,
> although I know I didn't do it right.

What are your specific concerns? Does the test run successfully?

> When is do_register_cleanup function called?

The "do_register_cleanup" call you added is executed during the "head.js" processing in the test harness, just before the test file. If you're interested in the details, that happens here:

https://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js#497

And this is what the function does:

https://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js#1064

> And how does returning the promise to its caller work out?

The termination callback function that you register is called by the test harness after all the tests in the file have finished. Again, if you're interested in the internal implementation details, this is the code that handles it:

https://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js#569

This basically delays the execution of the next test or termination callback until the Promise it returns is resolved. Note how the test harness invokes the "then" function on the returned Promise.

See this page if you're unfamiliar with Promises:

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

I think you could rewrite the code inside the termination callback using the "new Promise()" constructor documented there.

Let me know if you have any other questions!

Comment 20

[Mrinal Dhar]

Attachment: Removes the need for tail.js by adding async callback in head.js

Hi, Paolo. Thank you for answering my questions. I've made the required changes in this patch, and the tests ran OK. Please review it and let me know if something else needs to be changed.

Comment 21

[:Paolo Amadini]

Comment on attachment 8672822 [details] [diff] [review]
Removes the need for tail.js by adding async callback in head.js

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

Thanks a lot! There is only a small change needed.

::: toolkit/components/jsdownloads/test/unit/head.js
@@ +828,5 @@
> +  // Ensure all the pending HTTP requests have a chance to finish.
> +    continueResponses();
> +  // Stop the HTTP server.
> +    gHttpServer.stop(resolve);
> +    resolve();

You have to remove this last call to "resolve()", since the "stop" function is supposed to call "resolve" itself when done. Otherwise, the test will finish before the HTTP server had a chance to stop completely. Note that this might not cause a visible issue or failure now, but might become apparent in the future.

Also, please adjust the indentation: the inner comments must be two spaces more indented, and the first comment should wrap at the 80 characters limit.

Looking forward to a new patch!

Comment 22

[Mrinal Dhar]

Thanks for the review. 

(In reply to :Paolo Amadini from comment #21)
> Comment on attachment 8672822 [details] [diff] [review]
> Removes the need for tail.js by adding async callback in head.js
> 
> Review of attachment 8672822 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks a lot! There is only a small change needed.
> 
> ::: toolkit/components/jsdownloads/test/unit/head.js
> @@ +828,5 @@
> > +  // Ensure all the pending HTTP requests have a chance to finish.
> > +    continueResponses();
> > +  // Stop the HTTP server.
> > +    gHttpServer.stop(resolve);
> > +    resolve();
> 
> You have to remove this last call to "resolve()", since the "stop" function
> is supposed to call "resolve" itself when done. Otherwise, the test will
> finish before the HTTP server had a chance to stop completely. Note that
> this might not cause a visible issue or failure now, but might become
> apparent in the future.

Yeah, I found the two mentions to "resolve" weird too, but for some reason, if I remove either one of those, the tests stop working. I understand that "stop" should call "resolve" once its done, but something seems to be wrong here. 

For example, if I don't send it to the stop function but call it in the end, the test fails. And if I don't call it in the end and only send it to the stop function, the test never ends and it gets stuck. Any idea about why that might be happening? 



> Also, please adjust the indentation: the inner comments must be two spaces
> more indented, and the first comment should wrap at the 80 characters limit.

Will do. 

> Looking forward to a new patch!

Comment 23

[:Paolo Amadini]

> And if I don't call it in the end and only send it to the
> stop function, the test never ends and it gets stuck. Any idea about why
> that might be happening?

Hm, maybe you can try to add some logging to see if the "stop" callback is actually called?

Maybe try like something like stop(function() { ...logging...; resolve(); });

You can also try to run individual test files instead of the whole directory. Maybe the simpler test files work and the ones with complex HTTP requests block the server for some reason?

Comment 24

[:Paolo Amadini]

And there is some info on httpd.js debugging here:

https://developer.mozilla.org/en-US/docs/Httpd.js/HTTP_server_for_unit_tests#Debugging_errors

Comment 25

[Mrinal Dhar]

I set the debug flag to true in httpd.js, which gave me this output on the console:

 0:06.42 PROCESS_OUTPUT: Thread-1 (pid:97915) "HTTPD-INFO | >>> stopping listening on port 51725"

... so we know the stop function *is* being called. I passed it an anon function that would log on the console, but it never prints anything and the test's just stuck after that line from the httpd.js debug. 
I also tried different test files, same result.

Comment 26

[:Paolo Amadini]

You may just have hit a deeper bug... unfortunately I don't have any more suggestions on how to proceed unless you want to debug httpd.js itself adding more debug output to it. I may find some time to help with this, but it would likely not happen before a few days from now.

Comment 27

[Mrinal Dhar]

(In reply to :Paolo Amadini from comment #26)
> You may just have hit a deeper bug... unfortunately I don't have any more

Should I file it as a new one on bugzilla? 

> suggestions on how to proceed unless you want to debug httpd.js itself
> adding more debug output to it. I may find some time to help with this, but
> it would likely not happen before a few days from now.

That's alright. In the meanwhile, I'll try to see how httpd.js is handling that stuff by adding debug messages here and there.

Comment 28

[:Paolo Amadini]

(In reply to Mrinal Dhar from comment #27)
> Should I file it as a new one on bugzilla? 

I suggest you do that only if you find the approximate reason and httpd.js needs changes, or alternatively if you want to create a minimal test case (could be a simplified xpcshell test creating a server and doing a "stop" call in a normal add_task test). Otherwise, I recommend spending the time on debugging :-)

Comment 29

[Patrick]

Is this bug still available. I would like to work on it

Comment 30

[Mrinal Dhar]

Hey, Patrick. 
I’m still on this one, though I require some help from Paolo debugging one of the files and he said he’d be available to help out soon. 

Mrinal

Comment 31

[:Paolo Amadini]

I've debugged the httpd.js issue and now understand what is happening.

Basically, we call do_get_profile() in the "head.js" file of download tests, and this registers a cleanup function that triggers the "profile-change-net-teardown" and "profile-change-teardown" observer notifications, where network connections are closed. This means that the next call to gHttpServer.stop in our own termination function will fail, because the server is already stopped.

If the entire do_register_cleanup call is placed inside test_common_initialize, which is called after the do_get_profile call, our cleanup function is invoked before the profile teardown (they're called in reverse order of registration), and the server can stop cleanly.

Mrinal, can you confirm that this solves the issue with removing the "resolve()" call?

Comment 32

[Mrinal Dhar]

Attachment: Put the do_register_cleanup call inside test_common_initialize

Hey Paolo, 
This seems to have fixed it. 

The tests ran successfully by putting the do_register_cleanup call inside the test_common_initialize function itself, like you suggested, after removing the extra call to resolve. I’ve attached a patch, hopefully it will be fine.

Comment 33

[:Paolo Amadini]

Comment on attachment 8699527 [details] [diff] [review]
Put the do_register_cleanup call inside test_common_initialize

Looks great, thanks! As a final improvement, I've moved the cleanup function registration closer to the code that starts the HTTP server:

https://hg.mozilla.org/try/rev/b3e28bc8b015#l1.8

I've removed the comment about using a Promise for the termination function from the code, since it should be a known technique anyways, and I've edited our general documentation wiki instead:

https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

This something I recommend doing as you find underdocumented techniques, it's quite easy and welcome by the MDN team.

This is the link to the tryserver build with the edited patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a4744bd7611

If tests are green, I'll land this in the next few days.

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5b9292b9f878

Comment 35

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8699527 [details] [diff] [review]
Put the do_register_cleanup call inside test_common_initialize

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

::: toolkit/components/jsdownloads/test/unit/xpcshell.ini
@@ +3,5 @@
>  tail =
>  skip-if = toolkit == 'android' || toolkit == 'gonk'
>  
>  # Note: The "tail.js" file is not defined in the "tail" key because it calls
>  #       the "add_test_task" function, that does not work properly in tail files.

Perhaps this comment could have been removed too?

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b9292b9f878