1413685 - Unify AsyncResource and Resource in sync resource.js.

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement)

Description

[Thom Chiovoloni [:tcsc] (ex-moco)]

In resource.js, we export both AsyncResource, and Resource, but these are the same class. There's a TODO in the code to unify these [0], but no related bug (so this is that).

It's worth noting that we have a number of tests that test the same functionality in Resource and AsyncResource separately, and we want to unify these as part of the same patch.

We can skip doing this if we decide on bug 1409534 first, but this sounds much easier (maybe even easy enough to be at the good-first-bug or good-second-bug level? but maybe not)

[0]: http://searchfox.org/mozilla-central/source/services/sync/modules/resource.js#351-353 added in bug 1355677

Comment 1

[Edouard Oger [:eoger]]

Attachment: Bug 1413685 - Unify AsyncResource and Resource.

Review commit: https://reviewboard.mozilla.org/r/195906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195906/

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8924674 [details]
Bug 1413685 - Unify AsyncResource and Resource.

https://reviewboard.mozilla.org/r/195906/#review201128

That was faster than I expected.

I feel like this isn't all of the tests I meant, but it might be, and anything else we can catch as we go.

Clearing review since even though my issues are small, since I suspect after going with Resource instead it will have fairly different code.

::: services/sync/modules/resource.js:5
(Diff revision 1)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -this.EXPORTED_SYMBOLS = [
> +this.EXPORTED_SYMBOLS = ["AsyncResource"];

Hmmm... I don't love that this now has a different name than the object it exports. (Sorry, I didn't consider this when filing the bug)

After discussion in IRC, we both agree that using Resource and not renaming the file is better.

::: services/sync/tests/unit/xpcshell.ini
(Diff revision 1)
>  run-sequentially = Restarts server, can't change pref.
>  tags = addons
>  [test_httpd_sync_server.js]
>  
>  # HTTP layers.
> -[test_resource.js]

Were there any tests in this file that aren't covered by test_resource_async.js?

Comment 3

[Edouard Oger [:eoger]]

Comment on attachment 8924674 [details]
Bug 1413685 - Unify AsyncResource and Resource.

https://reviewboard.mozilla.org/r/195906/#review201128

> Were there any tests in this file that aren't covered by test_resource_async.js?

Yep, and they are also well separated.

Comment 4

[Edouard Oger [:eoger]]

Comment on attachment 8924674 [details]
Bug 1413685 - Unify AsyncResource and Resource.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195906/diff/1-2/

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8924674 [details]
Bug 1413685 - Unify AsyncResource and Resource.

https://reviewboard.mozilla.org/r/195906/#review201146

Thanks, looks great!

::: services/sync/tests/unit/test_resource.js:31
(Diff revision 2)
>  }
>  
>  function server_protected(metadata, response) {
>    let body;
>  
> -  if (has_hawk_header(metadata)) {
> +  if (has_hawk_header(metadata, "guest", "guest")) {

I don't think this function takes 3 arguments? http://searchfox.org/mozilla-central/source/services/sync/tests/unit/head_http_server.js#34

::: services/sync/tests/unit/test_resource.js:161
(Diff revision 2)
>  
> -  let logger = Log.repository.getLogger("Test");
> +function run_test() {
> +  logger = Log.repository.getLogger("Test");
>    Log.repository.rootLogger.addAppender(new Log.DumpAppender());
>  
> +  Svc.Prefs.set("network.numRetries", 1); // speed up test

Oh, hm. Is there a reason not to add this at the top level in a head file? Or would it not make a difference. (I'm wondering if we could avoid some rare timeout oranges on oour longer tests this way -- like test_errorhandlerN)

::: services/sync/tests/unit/test_resource.js:597
(Diff revision 2)
> -  res18._log.warn = oldWarn;
> +add_task(async function test_timeout() {
> -
>    _("Ensure channel timeouts are thrown appropriately.");
>    let res19 = new Resource(server.baseURI + "/json");
>    res19.ABORT_TIMEOUT = 0;
> -  error = undefined;
> +  await Assert.rejects(res19.get(), error => {

Whoa, Assert.rejects is much more convenient to use than passing a message regexp to Assert.throws (I guess it occurs to me now that maybe Assert.throws will allow a function also, but I've never checked).

::: services/sync/tests/unit/xpcshell.ini
(Diff revision 2)
>  tags = addons
>  [test_httpd_sync_server.js]
>  
>  # HTTP layers.
>  [test_resource.js]
> -[test_resource_async.js]

Mozreview won't let me delete this comment, or I would (since I'm pretty sure it's fine). But since it won't, I might as well ask it.

Just to be 100% clear, that file doesn't test anything that isn't now covered by some other test?

My concern is only that we don't accidentally reduce test coverage -- I'm pretty sure what you have is fine.

Comment 6

[Edouard Oger [:eoger]]

Comment on attachment 8924674 [details]
Bug 1413685 - Unify AsyncResource and Resource.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195906/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be25d3b0603a
Unify AsyncResource and Resource. r=tcsc

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/be25d3b0603a