Turn test warnings into test failures

NEW
Assigned to

Status

()

P3
enhancement
4 years ago
2 years ago

People

(Reporter: dylan, Assigned: dylanAtHome)

Tracking

Details

(Reporter)

Description

4 years ago
Right now, at least in 012throwables.t, there is a category of "Warnings" which are tests that "fail", except don't. In BMO, we're in the habit (at least, some of us) of modifying this test so that the warning is an error. Having to modify a test is really not a great thing.

I personally think the idea of a "warning" from a unit test to be contrived, but to continue the compromise it should be conditional on an ENV variable.

@dkl: for continuous integration, do we currently run with these warnings enabled? (probably not, right?)
Flags: needinfo?(dkl)
(In reply to Dylan William Hardison [:dylan] from comment #0)
> @dkl: for continuous integration, do we currently run with these warnings
> enabled? (probably not, right?)

on tinderbox those sorts of warnings would result in a failure.
(Reporter)

Comment 2

4 years ago
Let's just make them test failures. If there are dynamically generated errors, we can add exceptions for them.
Flags: needinfo?(dkl)
Summary: Add optional ENV variable to turn test warnings into test failures → Turn test warnings into test failures

Comment 3

4 years ago
I don't see why warnings should be failures. Warnings made Tinderbox to turn orange, while errors made Tinderbox to turn red. If you kept an extra error code which is now unused, this is a warning, because Bugzilla will still work. If you forgot to add an error code, this is a failure, because Bugzilla won't work without it. If you complain is only about 012throwables.t, then simply replace --WARNING by --ERROR and that's it.
(In reply to Frédéric Buclin from comment #3)
> I don't see why warnings should be failures. Warnings made Tinderbox to turn
> orange, while errors made Tinderbox to turn red.

tinderbox reported 012's warnings as "test failures".
http://logs.glob.uno/?c=mozilla%23bugzilla&s=25+Nov+2013&e=25+Nov+2013#c1308604
but that isn't important as we don't use tinderbox.

> If you kept an extra error code which is now unused, this is a warning, because Bugzilla will still work.

we already have a lot of tests that cause runtests to report FAIL but do not impact bugzilla's functionality (eg. whitespace, spelling, missing pod docs for subs, ...).  these sorts of "code correctness" tests are still important, and should continue to trigger failures.

> If you complain is only about 012throwables.t, then simply replace --WARNING by --ERROR and that's it.

actually the problem is it's reporting test-passed with --WARNING, while all our other warnings are test-failed with --WARNING.  ie. change ok(1, ..) to ok(0, ..).


given the boolean nature of modern CI systems, imho we should:
- change "failed with warning" to "failed" by removing "--WARNING" from the failure reason text
- change 012's "passed with warning" to "failed"
(Reporter)

Updated

4 years ago
Severity: normal → enhancement
Priority: -- → P3
(Reporter)

Updated

2 years ago
Assignee: dylan → dylan
You need to log in before you can comment on or make changes to this bug.