720596 - Ensure all test code sets clusterURL and serverURL

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

A bunch of test code (e.g. test_syncengine_sync.js) sets only the clusterURL pref and not serverURL. They should both be set in nearly every scenario.

Comment 1

[Murali]

Hi, I would like to work on this bug. Can you please guide me? I am new here.

Comment 2

[Gregory Szorc [:gps]]

(In reply to Murali from comment #1)
> Hi, I would like to work on this bug. Can you please guide me? I am new here.

Yes! Sorry I missed your IRC message, as I was sleeping. But, I see philikon helped you in the #sync channel.

The gist of this bug is that in a number of places throughout Sync's unit test code (called xpcshell tests), we are setting the "services.sync.clusterURL" or "services.sync.serverURL" Firefox preference but not both.

This code typically looks like:

  Svc.Prefs.set("clusterURL", "http://localhost:8080/");

We want:

  Svc.Prefs.set("clusterURL", "http://localhost:8080/");
  Svc.Prefs.set("serverURL", "http://localhost:8080/");

All the files that need changed are in the /services/sync/tests/unit/ directory of a services-central repository clone. I /think/ you got far enough with philikon over IRC to know what I am talking about. If you need more guidance, let me know.

Comment 3

[Murali]

Okay, I think I understand what needs to be done here. I'm working on it now. Thank you so much!

Comment 4

[Gregory Szorc [:gps]]

There's a patch in bug 710448 to add the following constants to a commonly-included JS file used by the tests:

  const TEST_CLUSTER_URL = "http://localhost:8080/";
  const TEST_SERVER_URL  = "http://localhost:8080/";

If you could use those constants instead of typing the same string/URL everywhere, that would be splendid. e.g.

  Svc.Prefs.set("clusterURL", TEST_CLUSTER_URL);

If you need to test, just add the above 2 lines to the top of /services/sync/tests/unit/head_http_server.js.

If you omit this, no big deal: we'll probably file a follow-up bug to bulk replace all the occurrences with the new constant.

Comment 5

[Murali]

Attachment: test code sets clusterURL and serverURL (using constants)

I used the constants as you said. Could you please check it and tell me whether any more changes are required?

Also, I am extremely sorry, I know I am supposed to follow the instructions at http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
before submitting a patch, I tried searching all over the place but I could not find the hgrc file mentioned there. So, I do not know to add my name at the top of the patch, etc. Could you please tell me what I might be missing? So Sorry to bother you.

Comment 6

[Josh Matthews [:jdm]]

https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F makes it clear that the file is ~/.hgrc. If it doesn't exist, you should create it.

Comment 7

[Gregory Szorc [:gps]]

(In reply to Murali from comment #5)
> Created attachment 591367 [details] [diff] [review]
> test code sets clusterURL and serverURL (using constants)
> 
> I used the constants as you said. Could you please check it and tell me
> whether any more changes are required?

Thanks for the patch! I'll probably take a look in a few hours. (I have semi-urgent patches that need to make it in the next Firefox and these are a higher priority.)

> Also, I am extremely sorry, I know I am supposed to follow the instructions
> at http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
> before submitting a patch, I tried searching all over the place but I could
> not find the hgrc file mentioned there. So, I do not know to add my name at
> the top of the patch, etc. Could you please tell me what I might be missing?
> So Sorry to bother you.

There's no need to apologize! The Mozilla development process is difficult. I was only in your shoes a few months ago. I wouldn't be where I am without asking tons of questions. We're here to help. So, don't feel bad about asking about anything.

If you can't figure out how to set up Mercurial, feel free to ping me on IRC. Also, if you ask in #introduction or #sync, you should get a response from *somebody*. If you still can't wrangle Mercurial to produce an ideal patch, you'll need to tell me the name and email you would like associated with the commit and I can format things on my end. But, I'd really like to see you get the Mercurial part down, as that is an important part of the Mozilla development process.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 591367 [details] [diff] [review]
test code sets clusterURL and serverURL (using constants)

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

This diff looks good and I can confirm the xpcshell tests pass on my machine.

While reviewing this, I realized there are 2 conventions within the test suite for modifying clusterURL and serverURL. One sets them directly on Service, the other through preferences. The difference is not subtle.

When going through Service., extra logic is invoked (see the implementation in service.js). Setting Service.serverURL directly even wipes out the clusterURL preference! So, if you do:

  Service.clusterURL = XXX;
  Service.serverURL = YYY;

that is wrong. Fortunately, we don't appear to listen for changes to the corresponding preferences, so setting the preferences (as opposed to the attributes on the Service object) in the "wrong" order (as I instructed you to do in this bug - oops) is OK.

As is often the case with Sync patches, the patch is alright, but there are definitely follow-up bugs to be filed.

Speaking of follow-ups, I noticed there are still a number of references to "http://localhost:8080/" remaining after this patch. Do you have any intention of addressing these? (I realize that this might be beyond the scope of what you signed up for, so it is OK to say no.) If you do though, please ensure the ordering sets serverURL *before* clusterURL.

The services-central tree is currently closed because we are preparing a merge to the main Firefox tree. So, I probably won't be committing this for a day or two.

Comment 9

[Murali]

Sure, I would like to work on this too. Can you please tell me whether the "http://localhost:8080/" should be replaced in all the other sync files?

Also, I will definitely change the order of setting Server and Cluster URL when I'm working on this.

Thank you so much for your help! :)

Comment 10

[Gregory Szorc [:gps]]

In the ideal world, "http://localhost:8080/" would only be defined in 1 place: the shared HTTP test header file, head_http_server.js.

Also, bug 710448 landed a few hours ago and that changes some of the files that you already updated. It also added the constants to head_http_server.js. If you haven't done it yet, please `hg pull -u` from services-central to ensure you have the latest file versions.

Comment 11

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #10)
> If you haven't done it yet, please `hg pull -u` from
> services-central to ensure you have the latest file versions.

Make sure to pop all of your patches first.

  hg qpop --all
  hg pull -u

Otherwise hg will helpfully merge your patch queue items and screw up your tree.

Comment 12

[Murali]

Attachment: Changed order of setting URL. Now clusterURL is not wiped. Removed http://localhost:8080/ from a lot of places

I've changed the order of setting clusterURL and serverURL so that the clusterURL is not wiped out. Also, I've replaced http://localhost:8080/ in a lot of places with either TEST_SERVER_URL or TEST_CLUSTER_URL.

There are still a lot of places with http://localhost:8080/

http://mxr.mozilla.org/mozilla-central/search?string=http://localhost:8080/&find=/services/sync/tests/unit/

Should I create a new constant in the header file for places like 

line 491 -- let res_headers = new AsyncResource("http://localhost:8080/headers");
line 503 -- let res_headers = new AsyncResource("http://localhost:8080/headers");

?


Thanks! :)

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 593862 [details] [diff] [review]
Changed order of setting URL. Now clusterURL is not wiped. Removed http://localhost:8080/ from a lot of places

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

Overall, the patch is very good! It is ready for commit except for the accidental inclusion of merge conflicts.

> There are still a lot of places with http://localhost:8080/
> Should I create a new constant in the header file for places like 
> 
> line 491 -- let res_headers = new
> AsyncResource("http://localhost:8080/headers");
> line 503 -- let res_headers = new
> AsyncResource("http://localhost:8080/headers");

The DRY principle says commonly reused strings like "http://localhost:8080/headers" should probably be turned into a constant. However, a lot of these repeated HTTP URIs are only used in 1 or 2 test files. So, I think it is acceptable to define the constants at the tops of the individual test files using them rather than make them global constants in head_http_server.js.

Also, I encourage you to learn how to submit Mercurial patches rather than the raw output from diff. I'm willing to cut you some slack for this bug. But, others are more stringent in their enforcement of the patch guidelines. https://developer.mozilla.org/en/Mercurial has a lot of information. If you need assistance, hop into the #introduction or #sync channels on IRC and someone (me if I see your request) will help.

::: services/sync/tests/unit/test_syncengine_sync.js
@@ +386,4 @@
>  add_test(function test_processIncoming_reconcile_local_deleted() {
>    _("Ensure local, duplicate ID is deleted on server.");
>  
> +<<<<<<< local

Looks like you accidentally picked up a merge conflict!

@@ +539,4 @@
>  add_test(function test_processIncoming_reconcile_changed_dupe() {
>    _("Ensure that locally changed duplicate record is handled properly.");
>  
> +<<<<<<< local

And another merge conflict.

Comment 14

[Gregory Szorc [:gps]]

As it turns out, this bug is important for supporting version 2.0 of the Sync protocol. We are starting to work on porting things to version 2.0, so the sooner we get this in, the better.

Murali: do you have an ETA on an updated patch?

Comment 15

[Murali]

I'll have it ready within the next 12 hours! :)

Comment 16

[Murali]

Attachment: Resolved Conflicts

I've resolved the conflicts. I'm declaring the constants at the top of 1 or 2 files. I'll have a newer patch including that up in the next half hour. Thanks! :)

Comment 17

[Murali]

Attachment: Patch

Could you please verify this patch and let me know whether there are any changes to be made? Thanks! :)

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 594388 [details] [diff] [review]
Patch

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

This is looking great! Every patch gets better.

My review comments mostly nitpicks. The only significant issue is the order of serverURL and clusterURL in test_errorhandler.js is incorrect.

::: services/sync/tests/unit/head_http_server.js
@@ -1,2 @@
>  const Cm = Components.manager;
> -

Remove introduced whitespace.

::: services/sync/tests/unit/test_errorhandler.js
@@ +121,5 @@
>    Service.username = "johndoe";
>    Service.password = "ilovejane";
>    Service.passphrase = "abcdeabcdeabcdeabcdeabcdea";
> +  Service.clusterURL = TEST_CLUSTER_URL;
> +  Service.serverURL  = TEST_SERVER_URL;

serverURL should be before clusterURL.

This is mixed up throughout this file. Please correct all instances.

::: services/sync/tests/unit/test_errorhandler_sync_checkServerError.js
@@ +50,5 @@
>  function setUp() {
>    Service.username = "johndoe";
>    Service.password = "ilovejane";
>    Service.passphrase = "aabcdeabcdeabcdeabcdeabcde";
> +  Service.clusterURL = TEST_CLUSTER_URL;

Please add Service.serverURL.

::: services/sync/tests/unit/test_history_engine.js
@@ +17,3 @@
>    Svc.Prefs.set("username", "foo");
>    Svc.Prefs.set("client.type", "mobile");
> +

Please remove whitespace change.

::: services/sync/tests/unit/test_interval_triggers.js
@@ +34,5 @@
>  function setUp() {
>    Service.username = "johndoe";
>    Service.password = "ilovejane";
>    Service.passphrase = "abcdeabcdeabcdeabcdeabcdea";
> +  Service.clusterURL = TEST_CLUSTER_URL;

Please add serverURL.

::: services/sync/tests/unit/test_resource_async.js
@@ +1,1 @@
> +

Remove whitespace change.

::: services/sync/tests/unit/test_resource_ua.js
@@ +1,1 @@
> +

Remove empty line at top of file.

::: services/sync/tests/unit/test_restrequest.js
@@ +1,5 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +
> +const TEST_RESOURCE_URL = TEST_RESOURCE_URL;

Remove empty line above const.

::: services/sync/tests/unit/test_score_triggers.js
@@ +45,5 @@
>  function setUp() {
>    Service.username = "johndoe";
>    Service.password = "ilovejane";
>    Service.passphrase = "sekrit";
> +  Service.clusterURL = TEST_CLUSTER_URL;

Set serverURL please.

::: services/sync/tests/unit/test_service_changePassword.js
@@ +25,5 @@
>      };
>    }
>  
>    try {
> +    Weave.Service.serverURL = TEST_SERVER_URL;

Missing clusterURL.

::: services/sync/tests/unit/test_service_getStorageInfo.js
@@ +12,5 @@
>  
>  function run_test() {
>    Service.username = "johndoe";
>    Service.password = "ilovejane";
> +  Service.clusterURL = TEST_CLUSTER_URL;

Need serverURL.

::: services/sync/tests/unit/test_service_sync_updateEnabledEngines.js
@@ +68,5 @@
>  function setUp() {
>    Service.username = "johndoe";
>    Service.password = "ilovejane";
>    Service.passphrase = "abcdeabcdeabcdeabcdeabcdea";
> +  Service.clusterURL = TEST_CLUSTER_URL;

Need serverURL.

::: services/sync/tests/unit/test_service_wipeServer.js
@@ +28,5 @@
>  
>  function setUpTestFixtures() {
>    let cryptoService = new FakeCryptoService();
>  
> +  Service.clusterURL = TEST_CLUSTER_URL;

missing serverURL

::: services/sync/tests/unit/test_syncengine_sync.js
@@ +1789,5 @@
>      SyncScheduler.hasIncomingItems = false;
>      Svc.Obs.remove("weave:engine:sync:applied", onApplied);
>    }
>  });
> +

Remove whitespace change.

::: services/sync/tests/unit/test_syncscheduler.js
@@ +51,5 @@
>  function setUp() {
>    Service.username = "johndoe";
>    Service.password = "ilovejane";
>    Service.passphrase = "abcdeabcdeabcdeabcdeabcdea";
> +  Service.clusterURL = TEST_CLUSTER_URL;

Missing serverURL.

@@ +666,5 @@
>    // it is not overwritten on sync:finish
>    let server = sync_httpd_setup();
>    setUp();
>  
> +  Service.serverURL = TEST_SERVER_URL;

Missing clusterURL.

Comment 19

[Murali]

Attachment: Updated Patch

Could you please tell me whether I need to modify this patch further? So Sorry to keep troubling you and thank you very much for your patience.

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 594517 [details] [diff] [review]
Updated Patch

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

Overall, just a few small nits remain. I will make these changes locally and commit the patch.

I had to remove the addition of serverURL from test_scheduler.js because it was breaking the test.

Comment 21

[Gregory Szorc [:gps]]

Pushed to services-central: https://hg.mozilla.org/services/services-central/rev/56a629990dc1

Marking as q- because all changes are to the test code and the changes are trivial in nature and don't change the semantics of the tests.

Murali: thank you very much for working on this! As I said earlier, this needed to get done to make the transition to supporting BrowserID easier and we truly appreciate the assist. I look forward to more contributions from you. If there is anything I can do to help you further, just ask.

Comment 22

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/56a629990dc1