Closed Bug 929149 Opened 6 years ago Closed 6 years ago

Reorganize test structure

Categories

(Toolkit :: Application Update, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

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

References

Details

Attachments

(13 files, 3 obsolete files)

6.26 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
4.11 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
1.25 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
4.52 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
69.54 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
84.49 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
280.10 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
1.16 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
1.15 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
84.59 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
11.02 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
297.08 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
767.54 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
There are several moz.build and Makefile.in files that are not needed and should be removed to speed up build times.
See https://groups.google.com/forum/#!topic/mozilla.dev.platform/F-uESi8ByPE

While removing these files the test_svc and test_timermanager directories should be moved into the same directory as the rest of the tests.

Since there will be a bunch of file renames needed it would be a "good thing" to give the xpcshell tests names that describe what they test.

Patches coming up
Comment on attachment 819951 [details] [diff] [review]
1. build changes

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

I didn't apply strict scrutiny. However, I imagine any regressions would be apparent from try run, etc.

::: toolkit/mozapps/update/tests/Makefile.in
@@ +79,5 @@
> +	$(INSTALL) $(srcdir)/data/simple.mar $(CHROMETESTROOT)/data
> +	$(INSTALL) $(srcdir)/data/simple_no_pib.mar $(CHROMETESTROOT)/data
> +	$(NSINSTALL) -D $(CHROMETESTROOT)/chrome
> +	$(PYTHON) $(MOZILLA_DIR)/config/Preprocessor.py -Fsubstitution $(DEFINES) $(ACDEFINES) $(srcdir)/chrome/utils.js > $(CHROMETESTROOT)/chrome/utils.js
> +	$(PYTHON) $(MOZILLA_DIR)/config/Preprocessor.py -Fsubstitution $(DEFINES) $(ACDEFINES) $(srcdir)/chrome/update.sjs > $(CHROMETESTROOT)/chrome/update.sjs

I know you are just copying these rules. But for future reference, we have INSTALL_TARGETS and PP_TARGETS to handle simple file copying and preprocessing more robustly.
Attachment #819951 - Flags: review?(gps) → review+
Comment on attachment 819951 [details] [diff] [review]
1. build changes

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

::: toolkit/mozapps/update/tests/moz.build
@@ +3,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +if CONFIG['MOZ_UPDATER']:

Why's that? It doesn't seem like we traverse into this dir otherwise.
I initially had the unit_timermanager in the same file... good catch
Attachment #819951 - Attachment is obsolete: true
Attachment #819951 - Flags: review?(netzen)
Attached patch 1. build changes rev2 (obsolete) — Splinter Review
gps, I implemented your review comments along with a few more and I'd like to make sure you are ok with it especially with the few more. Thanks!
Attachment #820710 - Flags: review?(gps)
Comment on attachment 820710 [details] [diff] [review]
1. build changes rev2

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

Looks good!

::: toolkit/mozapps/update/tests/Makefile.in
@@ +28,5 @@
> +
> +INSTALL_TARGETS       += xpcshell-data
> +xpcshell-data_TARGET  := libs
> +xpcshell-data_DEST    := $(XPCSHELLTESTROOT)/data
> +xpcshell-data_FILES   := $(wildcard $(srcdir)/data/*)

You can't express this with support-files in the .ini files? If not, we should file a bug so we can better express what you need to do in moz.build/manifest files.
Attachment #820710 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #18)
> Comment on attachment 820710 [details] [diff] [review]
> 1. build changes rev2
> 
> Review of attachment 820710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> ::: toolkit/mozapps/update/tests/Makefile.in
> @@ +28,5 @@
> > +
> > +INSTALL_TARGETS       += xpcshell-data
> > +xpcshell-data_TARGET  := libs
> > +xpcshell-data_DEST    := $(XPCSHELLTESTROOT)/data
> > +xpcshell-data_FILES   := $(wildcard $(srcdir)/data/*)
> 
> You can't express this with support-files in the .ini files? If not, we
> should file a bug so we can better express what you need to do in
> moz.build/manifest files.
No I can't since it is in the parent directory. The data files are used by all 3 sets of unit tests so I went with putting it in the parent dir where all 3 can access them instead of copying them into all 3 unit test directories. Also, a couple of the files are shared by the chrome tests. I think this is more of a one-off that is better expressed this way though the same end result could be achieved if we allowed manifests that didn't require tests.

Also, would it be a good thing to add the ability to preprocess in the manifests? That way it would be possible to invoke the tests and update the preprocessed test files before running the tests similar to how it updates the test files themselves.
(In reply to Robert Strong [:rstrong] (do not email) from comment #19) 
> Also, would it be a good thing to add the ability to preprocess in the
> manifests? That way it would be possible to invoke the tests and update the
> preprocessed test files before running the tests similar to how it updates
> the test files themselves.

Not sure I like that idea, sometimes it's important to see what you aren't running as well. What would be the use cases?
(In reply to Andrew Halberstadt [:ahal] from comment #20)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #19) 
> > Also, would it be a good thing to add the ability to preprocess in the
> > manifests? That way it would be possible to invoke the tests and update the
> > preprocessed test files before running the tests similar to how it updates
> > the test files themselves.
> 
> Not sure I like that idea, sometimes it's important to see what you aren't
> running as well. What would be the use cases?

(In reply to Robert Strong [:rstrong] (do not email) from comment #19)
>...
> That way it would be possible to invoke the tests and update the
> preprocessed test files before running the tests similar to how it updates
> the test files themselves.
btw: I am already used to calling make, make.py, mozmake, etc. to update all of the tests before running them. I don't have a problem with mach updating the tests though I think if it is going to update regular files it would make sense if it also updated preprocessed files.
Attachment #819953 - Flags: review?(netzen) → review+
Attachment #819954 - Flags: review?(netzen) → review+
Comment on attachment 820710 [details] [diff] [review]
1. build changes rev2

Brian, it would be good to have you give this a once over as well.
Attachment #820710 - Flags: review?(netzen)
Attachment #819958 - Flags: review?(netzen) → review+
Comment on attachment 819960 [details] [diff] [review]
5. move shared js

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

self note: uses gURLData from part 7
Attachment #819960 - Flags: review?(netzen) → review+
Attachment #820026 - Flags: review?(netzen) → review+
Attachment #820021 - Flags: review?(netzen) → review+
Comment on attachment 820022 [details] [diff] [review]
7. move / cleanup aus unit tests

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

::: toolkit/mozapps/update/tests/unit_aus_update/head_update.js
@@ +128,5 @@
>  // Use a copy of the main application executable for the test to avoid main
>  // executable in use errors.
>  const FILE_WIN_TEST_EXE = "_aus_test_app.exe";
>  
> +var gURLData;

nit: no need, already defined

@@ +347,5 @@
> +    do_throw("should only be called once!");
> +  }
> +
> +  let caller = Components.stack.caller;
> +  gTestID = caller.filename.toString().split("/").pop().split(".")[0];

much better
Attachment #820022 - Flags: review?(netzen) → review+
(In reply to Robert Strong [:rstrong] (do not email) from comment #21)
> > That way it would be possible to invoke the tests and update the
> > preprocessed test files before running the tests similar to how it updates
> > the test files themselves.

I think I misread your original comment and thought you meant preprocessing the manifest files themselves. If that's the case, carry on.
(In reply to Brian R. Bondy [:bbondy] from comment #25)
> Comment on attachment 820022 [details] [diff] [review]
> 7. move / cleanup aus unit tests
> 
> Review of attachment 820022 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/tests/unit_aus_update/head_update.js
> @@ +128,5 @@
> >  // Use a copy of the main application executable for the test to avoid main
> >  // executable in use errors.
> >  const FILE_WIN_TEST_EXE = "_aus_test_app.exe";
> >  
> > +var gURLData;
> 
> nit: no need, already defined
Fixed

Also made the test server port local to the function since it is no longer used outside of the function.

Carrying forward r+

Note: the build changes patch was very slightly bitrotted by bug 929905 and I'll submit an updated version after bug 929905 makes it to m-c
Attachment #822079 - Flags: review+
Attachment #820023 - Flags: review?(netzen) → review+
Attachment #820024 - Flags: review?(netzen) → review+
Attachment #820027 - Flags: review?(netzen) → review+
Comment on attachment 820710 [details] [diff] [review]
1. build changes rev2

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

I don't understand a lot of thee build config changes, but from what I do understand, I don't see any problems.
Attachment #820710 - Flags: review?(netzen) → feedback+
Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/8f84fb609a5e
Status: NEW → ASSIGNED
Flags: in-testsuite+
Target Milestone: --- → mozilla27
https://hg.mozilla.org/mozilla-central/rev/8f84fb609a5e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.