Remove cpp unittests from 'make check'

RESOLVED FIXED in mozilla29

Status

()

Core
Build Config
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dminor, Assigned: dminor)

Tracking

(Blocks: 1 bug)

unspecified
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Now that we have cpp unittests running from the test package, they can be removed from make check.
(Assignee)

Updated

4 years ago
Depends on: 949538
(Assignee)

Comment 1

4 years ago
Created attachment 8357147 [details] [diff] [review]
Remove cpp unittests from make check

Is there anything else that needs to be done before we can turn these off?
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Attachment #8357147 - Flags: feedback?(gps)
Attachment #8357147 - Flags: feedback?(emorley)

Comment 2

4 years ago
Comment on attachment 8357147 [details] [diff] [review]
Remove cpp unittests from make check

I'm not sure sorry - ted should know though :-)
Attachment #8357147 - Flags: feedback?(emorley) → feedback?(ted)
(Assignee)

Comment 3

4 years ago
(In reply to Ed Morley [:edmorley UTC+0] from comment #2)
> Comment on attachment 8357147 [details] [diff] [review]
> Remove cpp unittests from make check
> 
> I'm not sure sorry - ted should know though :-)

Ed - no objections to turning these off soon then?

Comment 4

4 years ago
If the standalone suite does everything that the tests running as part of make check do, and the removal of this one make check target doesn't break anything from a releng POV (not sure what targets they run manually); then I'm really looking forward to this landing! :-) (Due to it saving cycles & reducing time before the build log ends up being uploaded & the build job visible in TBPL)
It will lose us our Win64 coverage on m-c, not that I actually care about that.

Comment 6

4 years ago
Comment on attachment 8357147 [details] [diff] [review]
Remove cpp unittests from make check

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

The build config patch looks good. I'm not the correct person to sign off on the policy change and am not sure who is. But I would think if we don't lose any test coverage, there should be no concerns about the policy.
Attachment #8357147 - Flags: feedback?(gps) → feedback+
(In reply to Phil Ringnalda (:philor) from comment #5)
> It will lose us our Win64 coverage on m-c, not that I actually care about
> that.

Are we not scheduling Win64 test jobs on m-c, or just not scheduling C++ unit test jobs? Either way, I suspect we can fix this in a followup.
Comment on attachment 8357147 [details] [diff] [review]
Remove cpp unittests from make check

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

Policy shmolicy, this is a build config patch and you're completely qualified to review it. dminor has done the due diligence here--the tests are running in a separate job, he added "mach cppunittests", we're in good shape. I'll post to dev.platform to let people know about the change, but I don't expect any issues.
Attachment #8357147 - Flags: feedback?(ted) → feedback+
We're neither scheduling the tests on m-c nor keeping slaves running to take the jobs if we scheduled them: the ec2 instances we use for Win64 tests just die after a few days, and we resurrect them after a few months when someone wants to look at whichever twig it is where we are hypothetically greening them up, Date I think.
(Assignee)

Comment 10

4 years ago
Comment on attachment 8357147 [details] [diff] [review]
Remove cpp unittests from make check

Thanks for the feedback.

I did a try run here: https://tbpl.mozilla.org/?tree=Try&rev=a530f9bd4138
Attachment #8357147 - Flags: review?(gps)

Updated

4 years ago
Attachment #8357147 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/cc8ef74976b1

\m/
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.