1014659 - Move cppunit tests to their own test package

Status: RESOLVED WONTFIX

(Testing :: General, defect)

Description

[Dan Minor [:dminor]]

We would eventually like to break up the test package into individual test suite packages to avoid overhead of downloading on unzipping the monolithic archive on each test machine (bug 917999).

A first step would be to move the cppunit tests into their own package. They are the largest component of the test archive and are likely to grow larger as we move more tests out of make check (bug 992323). For instance, removing gtest from make check requires adding the gtest libxul to the test package which is 80+ MB on Linux.

Comment 1

[Dan Minor [:dminor]]

Attachment: Move cppunit tests to their own test package

This special cases the cppunit tests and moves them into their own test package.

We might eventually want to move every test type into individual packages, but I thought there was enough benefit in getting the cppunit tests out of the main test package that it was worthwhile doing that prior to coming up with a generic solution that handles each test type.

This will require mozharness changes before it can be landed, but I wanted to hold off on that until it looked like this approach would be ok.

My thought for mozharness would be to download the main test package and then attempt to download the cppunit test package if we are running those tests. It would not be an error if the cppunit test package is not present since we wouldn't expect these changes to be present on every branch initially.

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8470219 [details] [diff] [review]
Move cppunit tests to their own test package

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

I don't have much opinion besides the fact that i really hate those rules, but you're merely inheriting what's already awful. Maybe ted has something to add.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 8470219 [details] [diff] [review]
Move cppunit tests to their own test package

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

This seems fine (given the existing awfulness). I assume you're going to have to make mozharness smart enough to fall back to the existing test package so you don't break things?

Comment 4

[Dan Minor [:dminor]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> Comment on attachment 8470219 [details] [diff] [review]
> Move cppunit tests to their own test package
> 
> Review of attachment 8470219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems fine (given the existing awfulness). I assume you're going to
> have to make mozharness smart enough to fall back to the existing test
> package so you don't break things?

I have a mozharness patch in progress to handle this, but I didn't want to waste someone's time reviewing that if my packaging changes weren't acceptable :)

Comment 5

[Dan Minor [:dminor]]

Attachment: Patch to attempt to download cppunit test package if available

Ash run here: https://tbpl.mozilla.org/?tree=Ash&rev=65b8285608ad

Comment 6

[Jordan Lund (:jlund)]

Comment on attachment 8479890 [details] [diff] [review]
Patch to attempt to download cppunit test package if available

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

lgtm :) this will be fantastic to have. My only concern is that ERROR_LEVEL=INFO seems to get ignored for at least some part of _download_unzip (see in line comment). I'll r+ so this isn't blocked but my gut says we should fix this soon. What do you think?

OOC - what branch or where will firefox-34.0a1.en-US.linux-i686.cppunit.tests.zip exist? Your ash jobs looks like it never exists but I suspect that is due to the pipes not being connected?

::: mozharness/mozilla/testing/testbase.py
@@ +229,3 @@
>          We should probably change some other methods to call this."""
>          dirs = self.query_abs_dirs()
>          zipfile = self.download_proxied_file(url, parent_dir=dirs['abs_work_dir'],

I _think_ this will work with proxxy. looking at https://bug1014659.bugzilla.mozilla.org/attachment.cgi?id=8479890 it just fails to find the url like the non proxy equivalent

::: scripts/android_panda.py
@@ +224,5 @@
> +                    # tests  might be packaged with the main test zip, so
> +                    # we don't halt on failure here.
> +                    try:
> +                        self._download_unzip(self.test_url.replace('tests',
> +                                             'cppunit.tests'),

should cppunit_test_url be configurable somehow instead of hard coded here?

@@ +226,5 @@
> +                    try:
> +                        self._download_unzip(self.test_url.replace('tests',
> +                                             'cppunit.tests'),
> +                                             dirs['abs_test_install_dir'],
> +                                             error_level=INFO)

it looks like INFO is being ignored for some part of this call. The build stays green but tbpl bolds and summarizes the error lines.


13:56:17     INFO - retry: Calling <bound method DesktopUnittest._download_file of <__main__.DesktopUnittest object at 0xa48be2c>> with args: ('https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/ash-linux/1409082447/firefox-34.0a1.en-US.linux-i686.cppunit.tests.zip', '/builds/slave/test/build/firefox-34.0a1.en-US.linux-i686.cppunit.tests.zip'), kwargs: {}, attempt #3
13:56:17  WARNING - Server returned status 404 HTTP Error 404: Not Found for https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/ash-linux/1409082447/firefox-34.0a1.en-US.linux-i686.cppunit.tests.zip
13:56:17    ERROR - Can't download from https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/ash-linux/1409082447/firefox-34.0a1.en-US.linux-i686.cppunit.tests.zip to /builds/slave/test/build/firefox-34.0a1.en-US.linux-i686.cppunit.tests.zip!
13:56:17    ERROR - Caught exception: HTTP Error 404: Not Found
13:56:17    ERROR - Caught exception: HTTP Error 404: Not Found
13:56:17    ERROR - Caught exception: HTTP Error 404: Not Found

::: scripts/desktop_unittest.py
@@ +411,5 @@
> +        except OSError:
> +            pass
> +
> +        if not os.path.isdir(abs_cppunittest_dir):
> +            self.log('cppunit test dir: %s does not exit' % abs_cppunittest_dir, level=FATAL)

I'm guessing this condition is good to have regardless of this patch? Or can it only be hit as a result of this patch?

also, self.fatal(msg) == self.log(msg, error_level=FATAL). w/e you prefer

Comment 7

[Dan Minor [:dminor]]

(In reply to Jordan Lund (:jlund) from comment #6)
> Comment on attachment 8479890 [details] [diff] [review]
> Patch to attempt to download cppunit test package if available
> 
> Review of attachment 8479890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm :) this will be fantastic to have. My only concern is that
> ERROR_LEVEL=INFO seems to get ignored for at least some part of
> _download_unzip (see in line comment). I'll r+ so this isn't blocked but my
> gut says we should fix this soon. What do you think?
> 
Thanks for catching that. It was hardcoded to ERROR in download_proxied_file in proxxy.py, so I've
changed that to use the passed in error level.

> OOC - what branch or where will
> firefox-34.0a1.en-US.linux-i686.cppunit.tests.zip exist? Your ash jobs looks
> like it never exists but I suspect that is due to the pipes not being
> connected?
> 
I'm planning to test my packaging change on Cedar once the mozharness patch lands. It is working fine locally, and I've done some try runs with it so I'm not expected problems, but you never know.

> ::: mozharness/mozilla/testing/testbase.py
> @@ +229,3 @@
> >          We should probably change some other methods to call this."""
> >          dirs = self.query_abs_dirs()
> >          zipfile = self.download_proxied_file(url, parent_dir=dirs['abs_work_dir'],
> 
> I _think_ this will work with proxxy. looking at
> https://bug1014659.bugzilla.mozilla.org/attachment.cgi?id=8479890 it just
> fails to find the url like the non proxy equivalent
> 
> ::: scripts/android_panda.py
> @@ +224,5 @@
> > +                    # tests  might be packaged with the main test zip, so
> > +                    # we don't halt on failure here.
> > +                    try:
> > +                        self._download_unzip(self.test_url.replace('tests',
> > +                                             'cppunit.tests'),
> 
> should cppunit_test_url be configurable somehow instead of hard coded here?
> 
The 'test_url' itself comes from buildbot. I think in the future when we break out each suite into individual test packages we'll want something in the config that tells us the test package name but I don't think we need this right now.

> @@ +226,5 @@
> > +                    try:
> > +                        self._download_unzip(self.test_url.replace('tests',
> > +                                             'cppunit.tests'),
> > +                                             dirs['abs_test_install_dir'],
> > +                                             error_level=INFO)
> 
> it looks like INFO is being ignored for some part of this call. The build
> stays green but tbpl bolds and summarizes the error lines.
> 
> 
> 13:56:17     INFO - retry: Calling <bound method
> DesktopUnittest._download_file of <__main__.DesktopUnittest object at
> 0xa48be2c>> with args:
> ('https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/ash-
> linux/1409082447/firefox-34.0a1.en-US.linux-i686.cppunit.tests.zip',
> '/builds/slave/test/build/firefox-34.0a1.en-US.linux-i686.cppunit.tests.
> zip'), kwargs: {}, attempt #3
> 13:56:17  WARNING - Server returned status 404 HTTP Error 404: Not Found for
> https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/ash-
> linux/1409082447/firefox-34.0a1.en-US.linux-i686.cppunit.tests.zip
> 13:56:17    ERROR - Can't download from
> https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/ash-
> linux/1409082447/firefox-34.0a1.en-US.linux-i686.cppunit.tests.zip to
> /builds/slave/test/build/firefox-34.0a1.en-US.linux-i686.cppunit.tests.zip!
> 13:56:17    ERROR - Caught exception: HTTP Error 404: Not Found
> 13:56:17    ERROR - Caught exception: HTTP Error 404: Not Found
> 13:56:17    ERROR - Caught exception: HTTP Error 404: Not Found
> 
> ::: scripts/desktop_unittest.py
> @@ +411,5 @@
> > +        except OSError:
> > +            pass
> > +
> > +        if not os.path.isdir(abs_cppunittest_dir):
> > +            self.log('cppunit test dir: %s does not exit' % abs_cppunittest_dir, level=FATAL)
> 
> I'm guessing this condition is good to have regardless of this patch? Or can
> it only be hit as a result of this patch?
With the changes in this patch I thought it would be good to have a specific check. This should only show up if the download fails.

> 
> also, self.fatal(msg) == self.log(msg, error_level=FATAL). w/e you prefer

Thanks for the review!

Comment 8

[Dan Minor [:dminor]]

Mozharness changes pushed to: https://hg.mozilla.org/build/mozharness/rev/279414b34e2c

Comment 9

[Chris Cooper [:coop] (he/him)]

Merged to production, and deployed.

Comment 10

[Dan Minor [:dminor]]

Attachment: Download separate cppunit test package if main test package does not contain it.

Sorry about the bustage on the last patch.

I think this is better - I only attempt to download the cppunit test package if the main test package is missing the cppunittest directory. This avoids having to change mozharness to allow for failed downloads.

Tested it on Ash here: https://tbpl.mozilla.org/?tree=Ash&rev=65b8285608ad

Comment 11

[Jordan Lund (:jlund)]

Comment on attachment 8484974 [details] [diff] [review]
Download separate cppunit test package if main test package does not contain it.

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

cool so IIUC we will only hit this when we remove cpp from test zip?

Comment 12

[Jordan Lund (:jlund)]

I should have caught that proxxy change. We don't want to fatal in proxy call as proxxy will have lots of urls to try and we should only fatal if error_level is fatal and all urls are exhausted. we had so many usw2 issues/bugs last week. hiding this in a condition will limit this to a limiting testing env so this is good

Comment 13

[Jordan Lund (:jlund)]

since this uses _download_unzip and that is hooked up to proxxy, we should inform catlee/Miroshnykov about this as it will be a new file and I don't know if this requires plumbing on the proxy server.

Comment 14

[Dan Minor [:dminor]]

catlee, gmiroshynkov is this likely to cause problems again?

Ted has been working on Bug 1059943 which would make this patch unnecessary. At this point, I'm happy to stop work on this patch rather than break more things.

Comment 15

[Dan Minor [:dminor]]

(In reply to Jordan Lund (:jlund) from comment #12)
> I should have caught that proxxy change. We don't want to fatal in proxy
> call as proxxy will have lots of urls to try and we should only fatal if
> error_level is fatal and all urls are exhausted. we had so many usw2
> issues/bugs last week. hiding this in a condition will limit this to a
> limiting testing env so this is good

No, that proxxy change was completely my fault. I was trying to address your comment about 'ERROR' showing up in the tbpl log and I only tested it locally since it seemed like a small change. I should have requested a new review and/or tested it on Ash before landing.

Comment 16

[George Miroshnykov ( :gmiroshnykov )]

Everything should work fine as long as error_level is not set to FATAL in self.download_file() inside proxxy.py

Hardcoding it to ERROR was kind of a hacky workaround because I wasn't brave / familiar enough to refactor mozharness to do this properly.

I guess we can replace ERROR with WARNING or something like that, as a single download timeout / failure inside proxxy is not that rare and we can recover from it anyway, but this is probably outside the scope of this bug.

Comment 17

[Dan Minor [:dminor]]

Thanks, pushed to: https://hg.mozilla.org/build/mozharness/rev/1cf1f407edf0

Comment 18

[Dan Minor [:dminor]]

Attachment: Use tests.cppunit instead of cppunit.tests

The last version of the patch doesn't work (see try run [1])

The problem is at [2], which causes the cppunit package to be downloaded rather than the main test package:
    if f['name'].endswith('tests.zip'):  # yuk

This patch uses 'tests.cppunit.zip' rather than 'cppunit.tests.zip' to avoid this. I didn't hit this in local testing because I was passing in the path to the tests zip on the command line.

I've also added code to download the separate package for the b2g emulator. The last patch added this accidentally for b2g_desktop, which doesn't currently have cppunit tests scheduled against it. I don't think there is any harm in leaving it in, but I can remove it if you prefer.

[1] https://tbpl.mozilla.org/?tree=Try&rev=f66af3d008bf
[2] mozharness/mozilla/testing/testbase.py#159

Comment 19

[Jordan Lund (:jlund)]

Comment on attachment 8490976 [details] [diff] [review]
Use tests.cppunit instead of cppunit.tests

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

lgtm :)

I guess the 'yuk' comment was foreshadowing

if we have no near intentions of doing cpp on b2g desktop, I think we should remove it suggests the script supports it.

Comment 20

[Dan Minor [:dminor]]

Thanks, pushed to: https://hg.mozilla.org/build/mozharness/rev/106d7e2467ab

Comment 21

[Dan Minor [:dminor]]

Attachment: Add cppunit to parse_make_upload

As can be seen in this try run [1], we're misidentifying the cppunit test package as the installer.

[1] https://tbpl.mozilla.org/?tree=Try&rev=c6a071e08f8a

Comment 22

[Jordan Lund (:jlund)]

Comment on attachment 8494679 [details]
Add cppunit to parse_make_upload

this *should* make the cppunit file go undetected from the build job and leave packageUrl to be the real package url.

Comment 23

[Dan Minor [:dminor]]

(In reply to Jordan Lund (:jlund) from comment #22)
> Comment on attachment 8494679 [details]
> Add cppunit to parse_make_upload
> 
> this *should* make the cppunit file go undetected from the build job and
> leave packageUrl to be the real package url.

Thanks, pushed to: https://hg.mozilla.org/build/buildbotcustom/rev/dae67de0a4dd

Comment 24

[Dan Minor [:dminor]]

Attachment: Fix cppunit destination dir for b2g emulator

I think I'm almost there. This try run is mostly good [1], but I used the wrong directory name for b2g emulator unittests.

[1] https://tbpl.mozilla.org/?tree=Try&rev=f0bca3be5a78

Comment 25

[Jordan Lund (:jlund)]

Comment on attachment 8496910 [details] [diff] [review]
Fix cppunit destination dir for b2g emulator

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

sorry for the delay on a minor patch

Comment 26

[Dan Minor [:dminor]]

(In reply to Jordan Lund (:jlund) from comment #25)
> Comment on attachment 8496910 [details] [diff] [review]
> Fix cppunit destination dir for b2g emulator
> 
> Review of attachment 8496910 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> sorry for the delay on a minor patch

No worries, pushed to https://hg.mozilla.org/build/mozharness/rev/213d460025e0

Comment 27

[Dan Minor [:dminor]]

As far as I can tell, the last remaining thing here is small change to the 'package-tests' step in b2g to copy the cppunit test package to the proper upload directory.

Pull request here: https://github.com/mozilla-b2g/gonk-misc/pull/199

Comment 28

[Dan Minor [:dminor]]

dhylands, can you review (or recommend a reviewer) for this pull request:
https://github.com/mozilla-b2g/gonk-misc/pull/199

Thank you!

Dan

Comment 29

[Dave Hylands [:dhylands]]

Dan. Normally you add an attachment either with the patch or use the "paste text as attachment" to include the URL of the pull request, and the reviewer can tag it as r+.

Anyways - your patch looks fine to me - so r+

Do you need me to merge the pull request?

Comment 30

[Dan Minor [:dminor]]

(In reply to Dave Hylands [:dhylands] from comment #29)
> Dan. Normally you add an attachment either with the patch or use the "paste
> text as attachment" to include the URL of the pull request, and the reviewer
> can tag it as r+.
> 
> Anyways - your patch looks fine to me - so r+
> 
> Do you need me to merge the pull request?

Thank you, I'll do that next time, I wasn't sure if b2g stuff was handled purely through github. Please merge my pull request. Thanks! Dan

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gonk-misc/commit/45c54a55e31758f7e54e5eafe0d01d387f35897a

Comment 32

[Dan Minor [:dminor]]

Still need to have the m-c patch reviewed and landed :)

Comment 33

[Dan Minor [:dminor]]

Attachment: Move cppunit tests to their own package

Try run here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d3380110eef1

Comment 34

[(not currently active) Ted Mielczarek]

Comment on attachment 8512810 [details] [diff] [review]
Move cppunit tests to their own package

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

::: testing/testsuite-targets.mk
@@ +419,5 @@
> +# But we still need to stage the cppunit tests because they live in a
> +# separate package.
> +CPP_PKG_STAGE = $(DIST)/universal/cpp-test-stage
> +package-tests: \
> +  stage-cppunittests

Maybe it doesn't matter, but this change will mean that the binaries in the cppunittest package won't be universal binaries on universal OS X builds. I guess we probably don't test on non-64-bit OS X nowadays anyway?

Comment 35

[Dan Minor [:dminor]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #34)
> Comment on attachment 8512810 [details] [diff] [review]
> Move cppunit tests to their own package
> 
> Review of attachment 8512810 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/testsuite-targets.mk
> @@ +419,5 @@
> > +# But we still need to stage the cppunit tests because they live in a
> > +# separate package.
> > +CPP_PKG_STAGE = $(DIST)/universal/cpp-test-stage
> > +package-tests: \
> > +  stage-cppunittests
> 
> Maybe it doesn't matter, but this change will mean that the binaries in the
> cppunittest package won't be universal binaries on universal OS X builds. I
> guess we probably don't test on non-64-bit OS X nowadays anyway?

I don't think it matters for running tests in automation, but now that you've pointed it out, it bothers me and I'll try to fix it.

Comment 36

[Dan Minor [:dminor]]

Attachment: Move cppunit tests to their own package

This handles the packaging for universal builds properly. Try run here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0fdc1ae972e4

Comment 37

[(not currently active) Ted Mielczarek]

Comment on attachment 8515951 [details] [diff] [review]
Move cppunit tests to their own package

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

Thanks! Sorry for the review delay.

Comment 38

[Dan Minor [:dminor]]

Looks like some more problems have shown up [1]:
- we're maybe not getting the cppunit test package on proxxy
- we're attempting to use the cppunit test package as the installer (again)
- and we're messing up the url for the jsshell package.

[1] https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2755232c7837

Comment 39

[Chris Manchester (limited bugmail, email directly)]

:dminor, are you planning to finish this off? Any reason it shouldn't be redone/undone with bug 917999?

Comment 40

[Dan Minor [:dminor]]

(In reply to Chris Manchester [:chmanchester] from comment #39)
> :dminor, are you planning to finish this off? Any reason it shouldn't be
> redone/undone with bug 917999?

My original intention was to start on bug 917999 here because the cppunit tests were the largest chunk of the tests zip and would give us the most benefit. With the new plans, I don't think there is any point continuing here.

Comment 41

[(not currently active) Ted Mielczarek]

attachment 8515951 [details] [diff] [review] is still probably useful as part of that work, but the rest of this should be subsumed by the more general work.