1136358 - Preprocessing and file includes make it difficult to debug app update tests

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

Patch coming up

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: 1. patch - file renames

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: patch - main

Try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b581c2e4ca7

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: 2. patch - build config

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: 3. patch - main

Comment 5

[Robert Strong (they/them - no direct email)]

try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0bf72da77d1

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 8569012 [details] [diff] [review]
2. patch - build config

I'll get a build peer to review this as well.

Comment 7

[Stephen A Pohl [:spohl]]

I'm missing some context here. Would you mind giving me a quick description of what the problem was with debugging app update tests and why these changes will make things better? Thanks!

Comment 8

[Robert Strong (they/them - no direct email)]

Line numbers are incorrect.

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 8569012 [details] [diff] [review]
2. patch - build config

Ted, this cleans up the Makefile.in a decent amount.

Comment 10

[Robert Strong (they/them - no direct email)]

Attachment: 4. patch - XPCOMUtils

along with some additional cleanup.

Comment 11

[Robert Strong (they/them - no direct email)]

Try run with all of the patches
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a2858549d89

Comment 12

[Robert Strong (they/them - no direct email)]

Attachment: 5. patch - fix responseXML exception

Comment 13

[Stephen A Pohl [:spohl]]

Comment on attachment 8569013 [details] [diff] [review]
3. patch - main

Review of attachment 8569013 [details] [diff] [review]:
-----------------------------------------------------------------

Phew, this was a large patch! :-) r=spohl with comments addressed/questions answered. Thanks!

::: toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js
@@ +11,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const DIR_MACOS = IS_MACOSX ? "Contents/MacOS/" : "";
> +const DIR_RESOURCES = IS_MACOSX ? "Contents/Resources/" : "";

Since we're changing this, can we use better variable names here? I think it's confusing to use Mac specific variables across platforms (especially in the consolidated GetMockUpdRootD() function). How about something like REL_GRE_BIN_DIR (instead of DIR_MACOS) and REL_GRE_DIR (instead of DIR_RESOURCES)? REL standing for "relative to the base/root dir" here. I'm not set on these particular names, so if you come up with better names I won't object.

@@ +12,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const DIR_MACOS = IS_MACOSX ? "Contents/MacOS/" : "";
> +const DIR_RESOURCES = IS_MACOSX ? "Contents/Resources/" : "";
> +const PLATSTRING =  + IS_MACOSX ? "_mac" : "";

I don't understand what "PLATSTRING" stands for (even though I understand its use below), or what the '+' at the beginning of the assignment is for. Could you explain?

::: toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedRecovery.js
@@ -162,1 @@
>            break;

nit: we could indent all the break; statements for better style

::: toolkit/mozapps/update/tests/unit_aus_update/updateManagerXML.js
@@ +11,5 @@
>                "prior to bug 530872");
>  
>    setUpdateChannel("test_channel");
>  
> +  let patch, patches, update, updates;

Rather than declaring the variables here, could you declare them where they're being defined (to match other files in this patch, like cleanupDownloadingForDifferentChannel.js)?

Comment 14

[Robert Strong (they/them - no direct email)]

(In reply to Stephen Pohl [:spohl] from comment #13)
> Comment on attachment 8569013 [details] [diff] [review]
> 3. patch - main
> 
> Review of attachment 8569013 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Phew, this was a large patch! :-) r=spohl with comments addressed/questions
> answered. Thanks!
> 
> ::: toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js
> @@ +11,5 @@
> > +
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +
> > +const DIR_MACOS = IS_MACOSX ? "Contents/MacOS/" : "";
> > +const DIR_RESOURCES = IS_MACOSX ? "Contents/Resources/" : "";
> 
> Since we're changing this, can we use better variable names here? I think
> it's confusing to use Mac specific variables across platforms (especially in
> the consolidated GetMockUpdRootD() function). How about something like
> REL_GRE_BIN_DIR (instead of DIR_MACOS) and REL_GRE_DIR (instead of
> DIR_RESOURCES)? REL standing for "relative to the base/root dir" here. I'm
> not set on these particular names, so if you come up with better names I
> won't object.
I'd prefer doing that in a followup

> 
> @@ +12,5 @@
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +
> > +const DIR_MACOS = IS_MACOSX ? "Contents/MacOS/" : "";
> > +const DIR_RESOURCES = IS_MACOSX ? "Contents/Resources/" : "";
> > +const PLATSTRING =  + IS_MACOSX ? "_mac" : "";
> 
> I don't understand what "PLATSTRING" stands for (even though I understand
> its use below), or what the '+' at the beginning of the assignment is for.
> Could you explain?
PLATSTRING was added to simplify things and I forgot to come up with a better name. I'll discuss a better one with you on irc.
The + is a typo

> :::
> toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedRecovery.js
> @@ -162,1 @@
> >            break;
> 
> nit: we could indent all the break; statements for better style
Will do.

> 
> ::: toolkit/mozapps/update/tests/unit_aus_update/updateManagerXML.js
> @@ +11,5 @@
> >                "prior to bug 530872");
> >  
> >    setUpdateChannel("test_channel");
> >  
> > +  let patch, patches, update, updates;
> 
> Rather than declaring the variables here, could you declare them where
> they're being defined (to match other files in this patch, like
> cleanupDownloadingForDifferentChannel.js)?
I did and then changed it back since it makes the lines go over 80... will do.

Comment 15

[Robert Strong (they/them - no direct email)]

Comment on attachment 8570611 [details] [diff] [review]
5. patch - fix responseXML exception

Broke something with this one

Comment 16

[Robert Strong (they/them - no direct email)]

Comment on attachment 8570611 [details] [diff] [review]
5. patch - fix responseXML exception

I'll fix this in a different bug

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8569012 [details] [diff] [review]
2. patch - build config

Review of attachment 8569012 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/update/tests/Makefile.in
@@ +4,5 @@
>  
>  XPCSHELLTESTROOT = $(abspath $(DEPTH))/_tests/xpcshell/$(relativesrcdir)
>  CHROMETESTROOT = $(abspath $(DEPTH))/_tests/testing/mochitest/chrome/$(relativesrcdir)
>  
> +PP_CONST_FILE = $(srcdir)/data/xpcshellConstantsPP.js

Nit: use a lowercase variable since UPPERCASE variables are by convention special (this standard isn't well enforced).

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: 2. patch - build config comments addressed

Comment 19

[Robert Strong (they/them - no direct email)]

Attachment: 3. patch - main comments addressed

Comment 20

[Robert Strong (they/them - no direct email)]

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/83dbba23976b
https://hg.mozilla.org/integration/fx-team/rev/0dd5fde46fb6
https://hg.mozilla.org/integration/fx-team/rev/bdf8e02fc055
https://hg.mozilla.org/integration/fx-team/rev/3339c7489564

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/83dbba23976b
https://hg.mozilla.org/mozilla-central/rev/0dd5fde46fb6
https://hg.mozilla.org/mozilla-central/rev/bdf8e02fc055
https://hg.mozilla.org/mozilla-central/rev/3339c7489564