1723189 - Keep a test configuration that runs without warnings-as-errors pref set

Status: NEW

(WebExtensions :: General, task, P3)

Description

[Tomislav Jovanovic :zombie]

Quoting Luca from bug 1722966 comment 12:

In my opinion, technically the android run is the one right, the test case is currently testing a test-only behavior instead of the behavior without that pref set as it was meant to do.

Only by accident did android xpcshell tests run without the pref set, and Rob says he's close to changing the situation on android.

That's a test that was testing the wrong thing (and passing) because the pref was changing to a test behavior. We're running most of our tests in a test-only configuration which we're not shipping.

This is bad. We should fix this.

We should have an explicit test configuration where we run code that we ship. This looks similar to various extra assertions we run in DEBUG builds (for example bug 1363886), so that would be ideal IMO, but I could also live with warnings-as-errors being off in debug builds if folks want the default test behavior to be with it on.

Comment 1

[Tomislav Jovanovic :zombie]

We should have an explicit test configuration where we run code that we ship. This looks similar to various extra assertions we run in DEBUG builds (for example bug 1363886), so that would be ideal IMO, but I could also live with warnings-as-errors being off in debug builds if folks want the default test behavior to be with it on.

Oh, I also remember us wanting to run return value assertions from bug 1363886 in local builds (somehow), even if it's an opt build, because that's where they make most sense, while implementing an API. warnings-as-errors seems like a similar thing we would want in local builds, and also in debug builds on Try as a safety check.

Comment 2

[Rob Wu [:robwu]]

(In reply to Tomislav Jovanovic :zombie from comment #0)

Quoting Luca from bug 1722966 comment 12:

In my opinion, technically the android run is the one right, the test case is currently testing a test-only behavior instead of the behavior without that pref set as it was meant to do.

Only by accident did android xpcshell tests run without the pref set, and Rob says he's close to changing the situation on android.

Yup, I'm going to fix the bug in bug 1723198.

You said that you'd like to block the bug (implying not landing the patch for that bug until this bug is resolved), but I added a see-also instead because the two issues are independent, and the other bug is serious enough to warrant landing sooner than later.

That's a test that was testing the wrong thing (and passing) because the pref was changing to a test behavior. We're running most of our tests in a test-only configuration which we're not shipping.

The purpose of this feature (introduced in bug 1570715) is to fail unit tests when a warning is triggered (e.g. the use of deprecated APIs), and could result in an observable difference between tests and reality when a warning is triggered in the schema, in the following cases:

Implementation	Intention of test author	Test expectation (code)	Reality	Result
no error	no errors	no error	no error	Good - Test fails unexpectedly during development with a useful error message. Dev updates patch before landing.
no error	expecting just a warning	promiseConsoleOutput	no error	Good - test fails unexpectedly during development with a useful error message. Dev updates patch before landing, hopefully by adding failOnSchemaWarnings(false) instead of adjusting the error message/skipping the test (not skipping the test would reveal bug 1723198).
no error	expecting just a warning	failOnSchemaWarnings(false) + promiseConsoleOutput	no error	Good - test matches intention
no error	expecting runtime error with warning message	WarningAsError	no error	Test does not match reality (control flow differs); should have used failOnSchemaWarnings(false) instead! Example of fix 1 in bug 1722966.
throws error	expecting different error	WarningAsError	actual error	Test does not match reality, but the control flow is consistent (with a different error). Test author should have compared result with expected error message. Example of fix 2 in bug 1722966.

We should have an explicit test configuration where we run code that we ship. This looks similar to various extra assertions we run in DEBUG builds (for example bug 1363886), so that would be ideal IMO, but I could also live with warnings-as-errors being off in debug builds if folks want the default test behavior to be with it on.

It's kinda nice to be able to use the pref on release, but on the other hand it's more common to have stricter checks in debug builds. Let's discuss during triage.

Comment 3

[Tomislav Jovanovic :zombie]

and the other bug is serious enough to warrant landing sooner than later.

You're implying this bug is not serous enough to warrant the same. I disagree.

How many times has somebody put up a patch that enables a pref in one of our tests because the change they're introducing is not compatible with extensions, and fails the test at question. We jump on those cases, demanding a different solution, because we don't want our tests testing something we're not shipping, even if it's just for a test or two.

If someone proposed a pref that would make all of our tests run in a configuration we're not shipping, we would not allow it.

Test does not match reality (control flow differs); should have used failOnSchemaWarnings(false) instead!

There is potential for a security-type error here. Suppose we want to prohibit a specific manifest or API key because it's not a safe configuration for something. We make the change, write a test that expects an error is thrown with that specific manifest/API param, and we're happy believing that configuration is now impossible, extension will never install/run if it uses that manifest/API key.

Except we made a mistake, it was only a warning instead of an error, but warnings-as-errors hid that fact, and now we have a bug. Depending on people to remember to use proper incantations, when it can be automated is foolish.

Comment 4

[Rob Wu [:robwu]]

(In reply to Tomislav Jovanovic :zombie from comment #3)

and the other bug is serious enough to warrant landing sooner than later.

You're implying this bug is not serous enough to warrant the same. I disagree.

Both bugs are serious (as they are both about running with unexpected prefs), but they can be resolved independently.

As mentioned in the table, the other bug (bug 1723198) would have been caught if the test_ext_schema.js test didn't get a skip-if, because an exact test case for this feature is part of https://searchfox.org/mozilla-central/rev/4b88e0b8cca115009e82fdd65e5bf5812ff99128/toolkit/components/extensions/test/xpcshell/test_ext_schema.js#36-50

When this bug is fixed (possibly by making the feature debug-only), then this test needs to be updated (to skip on builds where the pref is disabled).

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Rob Wu [:robwu] from comment #4)

(In reply to Tomislav Jovanovic :zombie from comment #3)

and the other bug is serious enough to warrant landing sooner than later.

You're implying this bug is not serous enough to warrant the same. I disagree.

Both bugs are serious (as they are both about running with unexpected prefs), but they can be resolved independently.

Personally I think I agree with Tomislav:

• without looking to the prefs actually set in testing/profiles/xpcshell/user.js, we may assume that in theory both bugs may have the same severity

• but if we look more closely to what are the prefs and which are the values being set on them in testing/profiles/xpcshell/user.js, then most of them looks like setting remote urls to dummy urls or disabling feature that would be unnecessary for unit tests covered by xpcshell test.

To be completely honest, I haven't double-checked each of those prefs and any possible side-effect, but at a first glance they don't seem to be the kind of prefs that I would expect a potential loss of test coverage from, as that "warnings-as-errors" pref this was going to in practice without Bug 1723198 (I wouldn't be surprised if those prefs may have triggered some intermittent failures in tests that were assuming those prefs were set as expected across all builds, but that wouldn't be a completely "silent" issue as this would have been).