1425544 - Sync tests should have all the logs enabled.

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

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

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 1

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

Attachment: Bug 1425544 - Ensure trace level logging is used throughout sync tests.

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

Comment 2

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 3

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

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

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

Comment 4

[Mark Hammond [:markh] [:mhammond]]

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?

Comment 5

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

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 6

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

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

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

Comment 7

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

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

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1cef3226f9a
Ensure trace level logging is used throughout sync tests. r=markh

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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