Closed Bug 992983 Opened 11 years ago Closed 9 years ago

Run gtest from test package rather than make check

Categories

(Testing :: General, defect)

Product:
Testing
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox46 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox46 --- fixed

People

(Reporter: dminor, Assigned: chmanchester)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files)

MozReview Request: imported patch dummy.patch
40 bytes, text/x-review-board-request
BenWa
: review+
Details
MozReview Request: Bug 992983 - Build and upload the gtest libxul during test packaging. r=ted
40 bytes, text/x-review-board-request
ted
: review+
Details
MozReview Request: Bug 992983 - Run gtests from mozharness' desktop_unittest.py r=jlund
40 bytes, text/x-review-board-request
jlund
: review+
Details
MozReview Request: Bug 992983 - Follow up to fix treatment of preflight methods in desktop_unittest.py r=jlund
40 bytes, text/x-review-board-request
jlund
: review+
Details
Followup to make BUILD_GTEST consistent with the gtest makefile ifdef
872 bytes, patch
chmanchester
: review+
Details | Diff | Splinter Review
We can probably add these to the cppunit job. We will need to package the gtest specific libxul.
Note glandium's bug 992323 comment 2, we currently link the gtest libxul binary on-demand during "make check", so we'll need to switch to doing that either during the build or during test packaging.
The gtest libxul binary (on linux64) is ~630MB unstripped, 70MB stripped, so this will have a big impact on test package size. We might not want to do this until we fix bug 917999.
Depends on: 1014659
Assignee: nobody → cmanchester
Bug 1185298 and bug 1185299 are XP only, so we don't need to address them to get to parity with the current tests (all the builders run the same OS, Windows Server 2008).
No longer depends on: 1185298, 1185299
See Also: → 1185298, 1185299
Bug 992983 - Build and upload the gtest libxul during test packaging.
Attachment #8647811 - Flags: review?(ted)
https://reviewboard.mozilla.org/r/16051/#review14383 ::: testing/mozharness/scripts/desktop_unittest.py:652 (Diff revision 1) > + # We can trust the return code of gtests (even on Windows), so we set > + # the status here regardless of the result of parsing test output. > + if return_code != 0: > + tbpl_status, log_level = TBPL_WARNING, WARNING This is unfortunate. An alternative would be to implement a destkop_unittest style summary in gtest. I don't think that would be a lot of work if this seems to ugly to live.
https://reviewboard.mozilla.org/r/16049/#review14385 ::: testing/testsuite-targets.mk:569 (Diff revision 1) > + $(MAKE) -C $(DEPTH)/testing/gtest gtest This is going to link the xul-gtest dll on windows PGO builds, which is going to essentially double to Windows PGO build times. See the comment in testing/gtest/Makefile.in
Comment on attachment 8647811 [details] MozReview Request: imported patch dummy.patch Bug 992983 - Build and upload the gtest libxul during test packaging.
Attachment #8647811 - Flags: review?(ted)
https://reviewboard.mozilla.org/r/16049/#review14385 > This is going to link the xul-gtest dll on windows PGO builds, which is going to essentially double to Windows PGO build times. See the comment in testing/gtest/Makefile.in I guess this patch needs to be re-worked so packaging the gtest xul can be turned off. I'll re-request review when it has.
https://reviewboard.mozilla.org/r/16051/#review14383 > This is unfortunate. An alternative would be to implement a destkop_unittest style summary in gtest. I don't think that would be a lot of work if this seems to ugly to live. I'm leaning towards this... the little tinderboxprint'd is somewhat helpful.
Attachment #8647811 - Attachment description: MozReview Request: Bug 992983 - Build and upload the gtest libxul during test packaging. → MozReview Request: Bug 992983 - Add test summary lines to gtest's automation-specific output format. r=benwa
Attachment #8647811 - Flags: review?(bgirard)
Comment on attachment 8647811 [details] MozReview Request: imported patch dummy.patch Bug 992983 - Add test summary lines to gtest's automation-specific output format. r=benwa
Comment on attachment 8647812 [details] MozReview Request: Bug 992983 - Build and upload the gtest libxul during test packaging. r=ted Bug 992983 - Build and upload the gtest libxul during test packaging. r=ted
Attachment #8647812 - Attachment description: MozReview Request: Bug 992983 - Run gtests from mozharness' desktop_unittest.py → MozReview Request: Bug 992983 - Build and upload the gtest libxul during test packaging. r=ted
Attachment #8647812 - Flags: review?(jlund) → review?(ted)
Bug 992983 - Run gtests from mozharness' desktop_unittest.py r=jlund
Attachment #8648311 - Flags: review?(jlund)
Comment on attachment 8647811 [details] MozReview Request: imported patch dummy.patch Bug 992983 - Add test summary lines to gtest's automation-specific output format. r=benwa
Comment on attachment 8647812 [details] MozReview Request: Bug 992983 - Build and upload the gtest libxul during test packaging. r=ted Bug 992983 - Build and upload the gtest libxul during test packaging. r=ted
Comment on attachment 8648311 [details] MozReview Request: Bug 992983 - Run gtests from mozharness' desktop_unittest.py r=jlund Bug 992983 - Run gtests from mozharness' desktop_unittest.py r=jlund
Comment on attachment 8647811 [details] MozReview Request: imported patch dummy.patch https://reviewboard.mozilla.org/r/16049/#review14515 Ship It!
Attachment #8647811 - Flags: review?(bgirard) → review+
Comment on attachment 8648311 [details] MozReview Request: Bug 992983 - Run gtests from mozharness' desktop_unittest.py r=jlund https://reviewboard.mozilla.org/r/16187/#review14547 this is impressive. I assume you must have run this locally as you seem to have covered all the angles. How would you like to try this out in automation? One branch, maybe cedar, than m-c wide?
Attachment #8648311 - Flags: review?(jlund) → review+
(In reply to Jordan Lund (:jlund) from comment #18) > Comment on attachment 8648311 [details] > MozReview Request: Bug 992983 - Run gtests from mozharness' > desktop_unittest.py r=jlund > > https://reviewboard.mozilla.org/r/16187/#review14547 > > this is impressive. I assume you must have run this locally as you seem to > have covered all the angles. How would you like to try this out in > automation? One branch, maybe cedar, than m-c wide? Thanks for the review. I've got it going on try, for instance: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6a6f2fd7a92&exclusion_profile=false
Comment on attachment 8647812 [details] MozReview Request: Bug 992983 - Build and upload the gtest libxul during test packaging. r=ted https://reviewboard.mozilla.org/r/16051/#review15847 ::: testing/gtest/Makefile.in:19 (Diff revision 3) > +endif Let's just rip out the `make check` bits entirely--there's already a `mach gtest` command, and that's what developers should be using. ::: testing/testsuite-targets.mk:427 (Diff revision 3) > - web-platform \ > + web-platform The $(NULL) is just there as a placeholder so you don't have to add/remove the line continuation when you add entries, you should just leave it. ::: testing/testsuite-targets.mk:573 (Diff revision 3) > + $(MAKE) -C $(DEPTH)/testing/gtest gtest I think we should probably just figure out how to build gtest as part of the build instead of during test packaging which is kind of weird (but not any weirder than building it during `make check` like we do now, honestly). This might make things complicated to fix stuff like bug 1055224. I'm fine with punting this to a followup to get this landed, just file it and note the bug number here in a FIXME comment. ::: toolkit/mozapps/installer/package-name.mk:141 (Diff revision 3) > +ifneq (1_WINNT,$(MOZ_PGO)_$(OS_ARCH)) I think it would be nicer to make this a standalone variable somewhere, like BUILD_GTEST.
Attachment #8647812 - Flags: review?(ted) → review+
https://reviewboard.mozilla.org/r/16049/#review15849 ::: testing/gtest/mozilla/GTestRunner.cpp:38 (Diff revision 3) > + printf("Failed: %d\n", aUnitTest.failed_test_count()); I wonder how hard it'd be to make gtests use structured logs?
Comment on attachment 8647811 [details] MozReview Request: imported patch dummy.patch Bug 992983 - Add test summary lines to gtest's automation-specific output format. r=benwa
Comment on attachment 8647812 [details] MozReview Request: Bug 992983 - Build and upload the gtest libxul during test packaging. r=ted Bug 992983 - Build and upload the gtest libxul during test packaging. r=ted
Comment on attachment 8648311 [details] MozReview Request: Bug 992983 - Run gtests from mozharness' desktop_unittest.py r=jlund Bug 992983 - Run gtests from mozharness' desktop_unittest.py r=jlund
Comment on attachment 8647812 [details] MozReview Request: Bug 992983 - Build and upload the gtest libxul during test packaging. r=ted Just a quick quick sanity check, I'm not entirely sure I addressed one of the previous comments in the intended fashion.
Attachment #8647812 - Flags: review+ → review?(ted)
https://reviewboard.mozilla.org/r/16051/#review16123 Drive-by review ::: toolkit/mozapps/installer/upload-files.mk:752 (Diff revision 4) > +ifdef BUILD_GTEST Thanks to wildcards, you don't need the condition here, so you can move the setting of BUILD_GTEST from config.mk to testsuite-target.mk.
Comment on attachment 8647811 [details] MozReview Request: imported patch dummy.patch Dummy commit so mozreview will show an interdiff.
Attachment #8647811 - Attachment description: MozReview Request: Bug 992983 - Add test summary lines to gtest's automation-specific output format. r=benwa → MozReview Request: Dummy commit so mozreview will show an interdiff.
Comment on attachment 8647812 [details] MozReview Request: Bug 992983 - Build and upload the gtest libxul during test packaging. r=ted Bug 992983 - Build and upload the gtest libxul during test packaging. r=ted
Comment on attachment 8648311 [details] MozReview Request: Bug 992983 - Run gtests from mozharness' desktop_unittest.py r=jlund Bug 992983 - Run gtests from mozharness' desktop_unittest.py r=jlund
Attachment #8647812 - Flags: review?(ted) → review+
Comment on attachment 8647812 [details] MozReview Request: Bug 992983 - Build and upload the gtest libxul during test packaging. r=ted https://reviewboard.mozilla.org/r/16051/#review16151
This has caused some widespread bustage.
We're running every preflight method independent of the suites specified -- some check whether there are relevant suites, others don't. I have a proper fix to only call preflight methods for specified suites I'll push to try when it opens.
Comment on attachment 8647811 [details] MozReview Request: imported patch dummy.patch imported patch dummy.patch
Attachment #8647811 - Attachment description: MozReview Request: Dummy commit so mozreview will show an interdiff. → MozReview Request: imported patch dummy.patch
Comment on attachment 8647812 [details] MozReview Request: Bug 992983 - Build and upload the gtest libxul during test packaging. r=ted Bug 992983 - Build and upload the gtest libxul during test packaging. r=ted
Comment on attachment 8648311 [details] MozReview Request: Bug 992983 - Run gtests from mozharness' desktop_unittest.py r=jlund Bug 992983 - Run gtests from mozharness' desktop_unittest.py r=jlund
Bug 992983 - Follow up to fix treatment of preflight methods in desktop_unittest.py r=jlund
Attachment #8658862 - Flags: review?(jlund)
(In reply to Chris Manchester [:chmanchester] from comment #36) > We're running every preflight method independent of the suites specified -- > some check whether there are relevant suites, others don't. I have a proper > fix to only call preflight methods for specified suites I'll push to try > when it opens. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d59bc5deafa
Comment on attachment 8658862 [details] MozReview Request: Bug 992983 - Follow up to fix treatment of preflight methods in desktop_unittest.py r=jlund https://reviewboard.mozilla.org/r/18709/#review16815 thanks for the fix!
Attachment #8658862 - Flags: review?(jlund) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d33dbac2f2d&exclusion_profile=false If all goes well with the windows tests on this push this is ready to land.
(In reply to Chris Manchester [:chmanchester] from comment #46) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=4d33dbac2f2d&exclusion_profile=false > > If all goes well with the windows tests on this push this is ready to land. That checks out. Will land this Monday.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c4d0768dd27d We're dumping symbols for the gtest libxul as a result of this patch, which is good, because stacks from failing gtests can get symbols now, but it's bad, because we're hitting Exception: Ambiguous symbol file for libxul.so in fix_stack_using_bpsyms.py
I don't see a convenient way to get a fuller path out of the symbols package. A quick fix would be to just skip the gtest libxul for now, but the -x argument to symbolstore.py only takes the basename into account. But it looks like changing from gtest/libxul.so to gtest/libxul-gtest.so doesn't break anything.
(In reply to Chris Manchester [:chmanchester] from comment #50) > But it looks like changing from gtest/libxul.so to gtest/libxul-gtest.so > doesn't break anything. Renaming the file is going to entirely skip the tests.
The tests run and pass from libxul-gtest.so, but as glandium pointed out on irc binary components could fail in circumstances I haven't thought of due to the hard-coded file name for libxul in nsXPCOMPrivate.h. Ted, do you have any advice on how we could deal with this (comment 49) from the symbols package?
Flags: needinfo?(ted)
(In reply to Chris Manchester [:chmanchester] from comment #52) > The tests run and pass from libxul-gtest.so, but as glandium pointed out on > irc binary components could fail in circumstances I haven't thought of due > to the hard-coded file name for libxul in nsXPCOMPrivate.h. It's not the only reason. The main reason is that binary components are linked against libxul, and will expect libxul to be loaded by the dynamic linker first. Which isn't a problem on linux, because it doesn't really care of the file name the library was loaded from (as long as its SONAME is the one expected by the binary component), but it definitely will be a problem on windows and maybe mac.
I think we can fix fix_stack_using_bpsyms to actually match symbols properly. The UUID that Breakpad uses is in the binary on all platforms. If we have the tools needed to read it out it should be fairly straightforward: $ readelf -n ../debug-mozilla-central/toolkit/library/libxul.so | grep "Build ID" Build ID: 91d9b0b504cd46224b3ce7069a271735992231b3 Mac: $ otool -l ../debug-mozilla-central/toolkit/library/XUL | grep uuid uuid CE4343B9-D4F9-3B90-878F-F3EC6E168FA7 Win: $ dumpbin -HEADERS ../debug-mozilla-central/toolkit/library/xul.dll | grep RSDS 554CD6FD cv 4F 078CE440 78CD240 Format: RSDS, {C7D95017-4631-4BB2-9415-9462EEBAF9D 5}, 1, c:\build\debug-mozilla-central\toolkit\library\xul.pdb ...if we don't have readelf/otool/dumpbin installed on our test machines then it'll be a little bit more of a PITA, but we can either write some Python or C++ to grab the value. We used to have one for Linux, it wouldn't be hard to write one that worked for all platforms: https://hg.mozilla.org/mozilla-central/annotate/f2253edf5fc4/toolkit/crashreporter/fileid/file_id.cc
Flags: needinfo?(ted)
No longer depends on: 1204148
I don't see exactly how, but is there a chance this might be causing the following OSX 10.10 opt mozmill failure in c-c? 13:21:45 INFO - ValueError: ('Missing distribution spec', '/builds/slave/test/build/tests/mozmill/resources/jsbridge') 13:21:45 INFO - Storing debug log for failure in /Users/cltbld/.pip/pip.log 13:21:45 WARNING - Return code: 2 13:21:45 FATAL - Could not install python package: /builds/slave/test/build/venv/bin/pip install --download-cache /builds/slave/test/build/venv/cache --timeout 120 --no-index --find-links http://pypi.pvt.build.mozilla.org.proxxy1.srv.releng.scl3.mozilla.com/pub --find-links http://pypi.pub.build.mozilla.org.proxxy1.srv.releng.scl3.mozilla.com/pub --find-links http://pypi.pvt.build.mozilla.org/pub --find-links http://pypi.pub.build.mozilla.org/pub /builds/slave/test/build/tests/mozmill/resources/jsbridge failed after 5 tries!
Flags: needinfo?(cmanchester)
Doesn't look like it, but if backing out the patch fixes it I'll take a look.
Flags: needinfo?(cmanchester)
This busted comm-central builds: make -C ./testing/gtest gtest make[1]: Entering directory `/builds/slave/tb-c-cen-l64-ntly-000000000000/build/objdir-tb/testing/gtest' make[1]: Nothing to be done for `/builds/slave/tb-c-cen-l64-ntly-000000000000/build/mozilla/testing/gtest/gtest'. make[1]: Leaving directory `/builds/slave/tb-c-cen-l64-ntly-000000000000/build/objdir-tb/testing/gtest' ./config/nsinstall -D dist/test-stage/gtest/gtest_bin cp -RL dist/bin/gtest dist/test-stage/gtest/gtest_bin cp: cannot stat `dist/bin/gtest': No such file or directory make: *** [stage-gtest] Error 1
Flags: needinfo?(cmanchester)
Adding the appropriate condition to testsuite-targets.mk[1] would keep us from attempting to package this on comm-central. [1] https://hg.mozilla.org/mozilla-central/file/3c6ab47a14b2/testing/testsuite-targets.mk#l416
Flags: needinfo?(cmanchester)
(In reply to Chris Manchester [:chmanchester] from comment #60) > Adding the appropriate condition to testsuite-targets.mk[1] would keep us > from attempting to package this on comm-central. Given https://hg.mozilla.org/mozilla-central/file/3c6ab47a14b2/testing/gtest/Makefile.in#l10, wouldn't the appropriate condition in https://hg.mozilla.org/mozilla-central/file/3c6ab47a14b2/testing/testsuite-targets.mk#l37 be ifneq (browser,$(MOZ_BUILD_APP)) BUILD_GTEST= endif rather than the current one? (Otherwise, I suppose one could add ifdef MOZ_THUNDERBIRD etc.)
Flags: needinfo?(cmanchester)
I suppose it would. Will fix in a follow up.
Flags: needinfo?(cmanchester)
Thanks! Here's a patch that might be enough, unless you want to modify other things too.
Attachment #8707604 - Flags: review?(cmanchester)
Assignee: cmanchester → aleth
Status: NEW → ASSIGNED
Comment on attachment 8707604 [details] [diff] [review] Followup to make BUILD_GTEST consistent with the gtest makefile ifdef Review of attachment 8707604 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me.
Attachment #8707604 - Flags: review?(cmanchester) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/589761e5c8173337e82b7635f7a42cc3bcc9884e Bug 992983 - Followup to make BUILD_GTEST consistent with the gtest makefile ifdef. r=chmanchester
Assignee: aleth → cmanchester
https://hg.mozilla.org/mozilla-central/rev/589761e5c817
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: