Closed Bug 969972 Opened 6 years ago Closed 6 years ago

browser_styleeditor_autocomplete.js will be permaorange when 29 merges to beta on March 17th


(DevTools :: Style Editor, defect)

Not set


(firefox27 unaffected, firefox28 unaffected, firefox29+ fixed, firefox30 fixed, firefox-esr24 unaffected, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 unaffected, b2g-v1.3 unaffected, b2g-v1.3T unaffected, b2g-v1.4 unaffected)

Firefox 30
Tracking Status
firefox27 --- unaffected
firefox28 --- unaffected
firefox29 + fixed
firefox30 --- fixed
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- unaffected


(Reporter: philor, Assigned: Optimizer)




(1 file)

Looks like the test thinks it knows how many things will be returned by autocomplete, when that depends on whether layout.css.font-features.enabled is true or not, and whether that's true or not depends on whether the RELEASE_BUILD define is set, and when we merge to beta and change the version number from 29.0a2 to 29.0, RELEASE BUILD gets set and you get permaorange like

Even without RELEASE_BUILD troubles (and that'll be true of pretty much every CSS prop that gets added, it'll be ifdef'ed off at first), expecting CSS hackers to know that along with needing to make their tests pass, there's a single browser-chrome test that thinks it knows how many exist and needs to be told that another exists seems pretty fragile.
The test tests that the number of autocomplete entries are not incorrect. How else should I test that ?
(Also, the same is true for various other autocomplete tests in markup view and rule view)
We used to have the same issue in debugger and console tests. It shouldn't test for the actual number of entries, since it's not supposed to verify the correctness of low-level APIs (we should, and hopefully do, have other tests for that). It should rather make sure that some well-known entries are present, or perhaps that the number of entries displayed on the popup matches the number of entries returned by the backend.
saw this called out in IRC by :philor about how it will turn perma-orange on beta on March 17 and thus will get turned off, and thus will not be able to catch regressions in any beta-uplifts related to this code.

Flag needinfo on the author of the test and everyone who reviewed any changes to this file. I suggest we get this fixed as a priority, and a good way to fix logic was presented in Oct as well
Flags: needinfo?(scrapmachines)
Flags: needinfo?(rcampbell)
Flags: needinfo?(mihai.sucan)
Flags: needinfo?(fayearthur)
* s/In October/2 weeks ago/  (bah at misreading number-only-date)
Assignee: nobody → scrapmachines
Flags: needinfo?(scrapmachines)
Flags: needinfo?(rcampbell)
Flags: needinfo?(mihai.sucan)
Flags: needinfo?(fayearthur)
Attached patch fixSplinter Review
Now using suggestions numbers by querying platform API instead if hard coded numbers.
try push :

Philor, can you push this through a similar try as in comment 0 to verify ?
Attachment #8384244 - Flags: review?(fayearthur)
Flags: needinfo?(philringnalda)
Attachment #8384244 - Flags: review?(fayearthur) → review+
Flags: needinfo?(philringnalda)
The above try is green too.
Keywords: checkin-needed

This is a test-only change, so it can land on Aurora without approval. Please put [checkin-needed-aurora] on the whiteboard after this gets merged to m-c and I'll take care of it :)
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Whiteboard: [checkin-needed-aurora]
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.