Closed Bug 1275081 Opened 3 years ago Closed 3 years ago

browser_rules_completion-new-property_02.js has a hardcoded expected number of "display" values, which is incompatible with having preffable "display" values

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: dholbert, Assigned: jdescottes)

References

()

Details

Attachments

(2 files)

Bug 1260419 has this line:
> 20   ["d", {}, "display", 1, 3, false],
> 21   ["VK_TAB", {}, "", -1, 41, true],
> 22   ["VK_DOWN", {}, "block", 0, 41, true],

As I understand it, "41" here is the number of expected values that the "display" property can take.   (That sounds like a lot, but it's including "initial" and "unset", and a bunch of moz-prefixed values, and all the various ruby & table parts, and a few webkit prefixed aliases.)

This is bad because:
 (1) Some of these display values (e.g. "grid", "inline-grid") are guarded by prefs which get switched off in release builds:
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js?rev=830a12b195de#2474
     Such values will require us to backport a hardcoded-number-fixup every merge day :-/ (as long as we have pref-controlled display values, at least).

 (2) Whenever we add new "display" values, we'll have to remember to update the hardcoded count in this one particular devtools test.

 (3) If those new "display" values are pref-controlled (and the prefs aren't shipping to release yet), they'll create more trouble for merge-day-fixup.

This is unsustainable; we shouldn't place this sort of a burden on CSS-engine hackers & merge-day sheriffs just so that this one random devtools test can have a hardcoded number.
I see several options towards a workable solution here:
 (A) We could make this test call pushPrefEnv to enable every pref-controlled "display" value that is enabled in Nightly builds.

 (B) We could make this test derive its expected number of values from property_database.js (which lives at layout/style/test/property_database.js).  That JS file has a record of all currently-supported values for all properties (and it checks prefs when it's populating itself, so that e.g. we'll only include "grid" as a display value if the grid pref is enabled.)

Option (A) would address my concerns (1) and (3) -- it would mean that, even after code rides the trains on merge day & values go from being default-enabled to default-disabled, this test would still enable them & see the same number of "display" values.

Route (B) would address all of my concerns, but I'm not sure if it's workable. (I'm not sure if we can reference layout/style/test/property_database.js from devtools tests.)

jdescottes, looks like you broadened this test to have this hardcoded "41" value here.  Could you weigh in here & perhaps take this bug?

(In the meantime, I'm running into this in bug 1274096, and I'm intending to simply bump the hardcoded "41" to "43" when I land that bug's patches.)
Flags: needinfo?(jdescottes)
(We already have a similar problem in a different devtools test, BTW -- inspector test "test_bug877690.html" [1] has its own private list of all-supported-display-values, which we have to update manually every time we add a new display value. One such test wasn't too much of a pain, but now that there are two devtools tests in this category, this is feeling unsustainable.)

[1] http://mxr.mozilla.org/mozilla-central/source/layout/inspector/tests/test_bug877690.html?force=1
(In reply to Daniel Holbert [:dholbert] from comment #0)
> Bug 1260419 has this line:
> > 20   ["d", {}, "display", 1, 3, false],
> > 21   ["VK_TAB", {}, "", -1, 41, true],
> > 22   ["VK_DOWN", {}, "block", 0, 41, true],

Sorry, I mis-pasted there -- I meant to say:
 "Bug 1260419 updated browser_rules_completion-new-property_02.js to have these lines:"
I guess my "merge day pain" concerns (numbered (1) and (3) in comment 0) are mitigated by the fact that we often unconditionally enable new display modes for testing, in http://mxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js .  In particular, we have the CSS Grid pref forced on there, so this won't actually cause trouble if CSS Grid gets default-disabled on merge days.

I'm still uncomfortable with concern (2), though -- I don't want the status quo to end up being "Hey, if you're going to add a new "display" value, make sure to update these N devtools tests (where recently we went from N=1 to N=2)
Even though I updated those tests when removing the limit of displayed suggestions, I totally agree they are really fragile and should be updated. Sorry for not tackling this at that time.

I am not convinced testing the number of displayed items and the index has any meaning since we don't mock or control the datasource we are using.

I would prefer to make the current tests less fragile by only checking if the autocomplete popup is opened/closed and if the expected suggestion is/is not in the list. And add another set of tests checking indexes & number of suggestions but using mocked lists of CSS properties / CSS values.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Flags: needinfo?(jdescottes)
Great, thanks for looking into this!  Glad we're in agreement that this is fragile & could be better. :)
Several devtools autocomplete tests are checking that the popup
displays an exact number of suggestion, and that the selected
suggestion is exactly at the expected index.

Since devtools do not control the list of CSS properties used for the
autocomplete, we should not assert anything based on it in tests.

Review commit: https://reviewboard.mozilla.org/r/55252/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55252/
Attachment #8756570 - Flags: review?(ttromey)
Attachment #8756571 - Flags: review?(ttromey)
In order to keep testing indexes & total number of displayed suggestions,
added two tests relying on mocked suggetions list.

The inplace-editor API has been modified to allow mocking the suggestions.

Helper methods in devtools/client/shared/test have been moved to a new
test helper helper_inplace_editor.js, loaded by all inplace-editor
tests of this test suite.

Review commit: https://reviewboard.mozilla.org/r/55254/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55254/
Comment on attachment 8756570 [details]
MozReview Request: Bug 1275081 - part1: remove hardcoded number of "displayed" suggestions in devtools tests;r=tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55252/diff/1-2/
Comment on attachment 8756571 [details]
MozReview Request: Bug 1275081 - part2: add inplace editor tests using mocked CSS property/values suggestions;r=tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55254/diff/1-2/
Comment on attachment 8756570 [details]
MozReview Request: Bug 1275081 - part1: remove hardcoded number of "displayed" suggestions in devtools tests;r=tromey

https://reviewboard.mozilla.org/r/55252/#review52216

Thank you.  Looks good.
Attachment #8756570 - Flags: review?(ttromey) → review+
Comment on attachment 8756571 [details]
MozReview Request: Bug 1275081 - part2: add inplace editor tests using mocked CSS property/values suggestions;r=tromey

https://reviewboard.mozilla.org/r/55254/#review52222

Thank you.  This looks good.
Attachment #8756571 - Flags: review?(ttromey) → review+
Comment on attachment 8756570 [details]
MozReview Request: Bug 1275081 - part1: remove hardcoded number of "displayed" suggestions in devtools tests;r=tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55252/diff/2-3/
Comment on attachment 8756571 [details]
MozReview Request: Bug 1275081 - part2: add inplace editor tests using mocked CSS property/values suggestions;r=tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55254/diff/2-3/
Thanks for the reviews Tom! Just rebased and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=356a7ddedbeb
Try is green, except for unrelated windows 7 clipboard test issues.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/48f6dbdcd9d2
https://hg.mozilla.org/mozilla-central/rev/ed8629e891d9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Depends on: 1277657
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.