Closed Bug 1425544 Opened 2 years ago Closed 2 years ago

Sync tests should have all the logs enabled.

Categories

(Firefox :: Sync, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: tcsc, Assigned: tcsc)

References

Details

Attachments

(1 file)

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.
Priority: -- → P2
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 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+
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)
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1cef3226f9a
Ensure trace level logging is used throughout sync tests. r=markh
https://hg.mozilla.org/mozilla-central/rev/f1cef3226f9a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.