Sync tests should have all the logs enabled.

RESOLVED FIXED in Firefox 59

Status

()

Firefox
Sync
P2
normal
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: tcsc, Assigned: tcsc)

Tracking

unspecified
Firefox 59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a month ago
At the moment we have some of them enabled, but most aren't set as trace logs, some logs are trace logs until we instantiate a new engine, and there are some logs (PlacesSyncUtils) that can't be enabled at trace level in tests without modifying the source of the module.

This is really quite annoying, and frequently causes debugging tests to take longer than it should.

Sadly, to do this we need to set both the logging prefs (for loggers that are not yet instantiated, or for ones that will be reset in e.g. the ones for sync engines) and the levels of the already instantiated loggers.
Comment hidden (mozreview-request)

Updated

a month ago
Depends on: 1425987
Priority: -- → P2

Comment 2

a month ago
mozreview-review
Comment on attachment 8937135 [details]
Bug 1425544 - Ensure trace level logging is used throughout sync tests.

https://reviewboard.mozilla.org/r/207836/#review214018

As we discussed, I think this will be much simpler with bug 1425987, so I'm clearing the review request. However, once this works correctly, I think we should remove all the existing logging hacks that tests do just to keep things clean and stop future cargo-culting of such hacks.
Attachment #8937135 - Flags: review?(markh)
Comment hidden (mozreview-request)

Comment 4

a month ago
mozreview-review
Comment on attachment 8937135 [details]
Bug 1425544 - Ensure trace level logging is used throughout sync tests.

https://reviewboard.mozilla.org/r/207836/#review214638

Looks great, thanks!

::: services/sync/modules-testing/utils.js:235
(Diff revision 2)
>      ns.Service.clusterURL = config.fxaccount.token.endpoint;
>    }
>  };
>  
> +function syncTestLogging(level = "Trace") {
> +  let logStats = initTestLogging();

most existing callers of this pass "Trace" - I don't think it matters because it's only used to the "test logger", but it probably can't hurt to keep passing Trace?
Attachment #8937135 - Flags: review?(markh) → review+
(Assignee)

Comment 5

a month ago
Nice catch, that was not intentional (doesn't seem to make a ton of difference, but there are probably logs we're missing without it)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

a month ago
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1cef3226f9a
Ensure trace level logging is used throughout sync tests. r=markh

Comment 9

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f1cef3226f9a
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.