Unify AsyncResource and Resource in sync resource.js.

RESOLVED FIXED in Firefox 58

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tcsc, Assigned: eoger)

Tracking

unspecified
Firefox 58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

2 years ago
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 hidden (mozreview-request)
Assignee

Updated

2 years ago
Assignee: nobody → eoger
Reporter

Comment 2

2 years ago
mozreview-review
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?
Attachment #8924674 - Flags: review?(tchiovoloni)
Assignee

Comment 3

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Reporter

Comment 5

2 years ago
mozreview-review
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.
Attachment #8924674 - Flags: review?(tchiovoloni) → review+
Comment hidden (mozreview-request)

Comment 7

2 years ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be25d3b0603a
Unify AsyncResource and Resource. r=tcsc
https://hg.mozilla.org/mozilla-central/rev/be25d3b0603a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.