Closed Bug 1445628 Opened 2 years ago Closed 2 months ago

Add an automated test for invalid list.json (to catch issues like bug 1445278)

Categories

(Firefox :: Search, enhancement, P5)

enhancement

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox61 --- affected

People

(Reporter: RyanVM, Unassigned)

References

Details

In bug 1445278, we silently broke Italian repacks on ESR52 due to an extraneous colon which wasn't caught by any of our automation. Ideally, we would have had a linter or automated test which was capable of spotting this problem when the commit first landed.
I took a look at eslint-plugin-json and jsonlint, neither of these would have picked up the issue that caused bug 1445278 as it was caused by something that is a valid key according to the json format.

Something that breaks the actual json file would be caught by those, but I expect that would also likely be caught with en-US tests as JSON.parse would have been broken (https://searchfox.org/mozilla-central/rev/8fa0b32c84f924c6809c690117dbd59591f79607/toolkit/components/search/nsSearchService.js#3448) - though this should probably be verified.
I think it would be helpful to have a sanity check for the JSON, e.g.
* JSON is valid.
* Only a specific set of keys are allowed (it would have caught this).
* Each locales needs a "default", and a "visibleDefaultEngines" nested into it.

Taking it a step further:
* Each searchplugin listed in list.json should map to an existing XML file in /searchplugins
(In reply to Francesco Lodolo [:flod] from comment #2)
> * Only a specific set of keys are allowed (it would have caught this).

OK, that might not be so straightforward, since we have regions and locales in the way.
Summary: Add an automated test which would have caught bug 1445278 → Add an automated test for invalid list.json (to catch issues like bug 1445278)
This error was caught by the python script that parsed this file, it just didn't break the build...
(In reply to Mike Kaply [:mkaply] from comment #4)
> This error was caught by the python script that parsed this file, it just
> didn't break the build...

Part of the problem here is when things are caught. I believe if that script result had been handled it would have only been caught when the nightly builds were run, and then there would need to be some archaeology to work out what broke it.

Catching things on developer's machines, or as soon as they try to land is much better and less expensive.
> Catching things on developer's machines, or as soon as they try to land is much better and less expensive.

Fair. It does have a standard grammar except for the locales part.
Priority: -- → P5

I just checked, the tests added in bug 1541196 & deps would have covered this, so I think we're probably good now.

Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.