Closed Bug 1136358 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: Application Update, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)

Details

Attachments

(4 files, 4 obsolete files)

Patch coming up
Attached patch 2. patch - build config (obsolete) — Splinter Review
Attachment #8568976 - Attachment is obsolete: true
Attached patch 3. patch - main (obsolete) — Splinter Review
Attachment #8568975 - Attachment description: patch - file renames → 1. patch - file renames
Attachment #8569012 - Attachment description: patch - build config → 2. patch - build config
Attachment #8569013 - Attachment description: patch - main → 3. patch - main
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #8568975 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8569012 [details] [diff] [review]
2. patch - build config

I'll get a build peer to review this as well.
Attachment #8569012 - Flags: review?(spohl.mozilla.bugs)
Attachment #8569013 - Flags: review?(spohl.mozilla.bugs)
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!
Flags: needinfo?(spohl.mozilla.bugs) → needinfo?(robert.strong.bugs)
Line numbers are incorrect.
Flags: needinfo?(robert.strong.bugs)
Attachment #8569012 - Flags: review?(spohl.mozilla.bugs) → review+
Attachment #8568975 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8569012 [details] [diff] [review]
2. patch - build config

Ted, this cleans up the Makefile.in a decent amount.
Attachment #8569012 - Flags: review?(ted)
along with some additional cleanup.
Attachment #8569637 - Flags: review?(spohl.mozilla.bugs)
Attachment #8570611 - Flags: review?(spohl.mozilla.bugs)
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)?
Attachment #8569013 - Flags: review?(spohl.mozilla.bugs) → review+
(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 on attachment 8570611 [details] [diff] [review]
5. patch - fix responseXML exception

Broke something with this one
Attachment #8570611 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8570611 [details] [diff] [review]
5. patch - fix responseXML exception

I'll fix this in a different bug
Attachment #8570611 - Attachment is obsolete: true
Attachment #8569637 - Flags: review?(spohl.mozilla.bugs) → review+
Attachment #8569012 - Flags: review?(ted) → review?(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).
Attachment #8569012 - Flags: review?(gps) → review+
Attachment #8569013 - Attachment is obsolete: true
Attachment #8570723 - Flags: review+
Attachment #8570723 - Attachment is patch: true
You need to log in before you can comment on or make changes to this bug.