1240964 - Addon validator complains about 'let' usage in the Addon SDK

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, defect)

Description

[Bobby Holley (:bholley)]

Attachment: Screen Shot 2016-01-19 at 4.12.21 PM.png

See the attached screenshot. This is totally not the developer's fault, and can be solved with a repack.

We need to dig in hear quickly and probably send out a retraction to the affected developers, since otherwise lots of people will waste time repacking / being upset / both.

Comment 1

[Bobby Holley (:bholley)]

Andy said he'd dig in here.

Comment 2

[Andy McKay]

It looks like the addon compat page shows around 1,011 add-ons that failed the validation bump. Compared with an average of under 100 each version. The last failure that was this high was around Firefox 27 and it looks like multiple attempts were made to fix that up.

This occurs because add-ons are bundling the SDK, but according to Kris we are actually ignoring the bundled SDK files and just using the ones from the tree. Is an appropriate plan here to whitelist the SDK bundles and re-run the validation? Not sure what jorgev and others do in this case.

Comment 3

[Kris Maglione [:kmag]]

Yeah, whitelisting them should be fine.

Comment 4

[Jorge Villalobos [:jorgev] (he/him)]

I didn't look at the distribution of validation failures, but there were other tests that also contributed to the higher-than-normal failure rate (__noSuchMethod__ and the DevTools module move, to be specific).

If the developers need to repack in order to solve the problem, I don't think ignoring the warning is the right solution. If we can repack them to fix the problem, that'd be best. Running the compat validation again would be confusing IMO.

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Andy McKay [:andym] from comment #2)
> This occurs because add-ons are bundling the SDK, but according to Kris we
> are actually ignoring the bundled SDK files and just using the ones from the
> tree.

Ah, I missed this part. If that's the case for all of these add-ons, then whitelisting makes sense. We can run the validation again to see how big of a difference it makes in terms of failures, and then decide if it's worth sending email to all affected developers again.

Comment 6

[Bobby Holley (:bholley)]

Ok. Please do so quickly, because delay here has a very concrete cost in addon developer goodwill.

Comment 7

[Jorge Villalobos [:jorgev] (he/him)]

I disagree on the urgency of this. The email makes it very clear that these are potential problems and the developer should verify them to see if they are relevant. Pretty much every compatibility test we run fails on the side of caution and false positives, and the impact to developers is minimal (they don't get further notifications if they don't take action but their add-ons continue to work).

Comment 8

[Bobby Holley (:bholley)]

The validation email says that the JS engine fails to parse the code in the addon SDK. The developer has no way of knowing that the sdk code within their addon is actually unused. My operating assumption after reading that email was that my addon was broken. The email reads as "Hey, your addon no longer works because we made some changes that broke some code you didn't write. Ball is in your court."

The best case we can hope for is that developer grudgingly spends an hour dusting off their SDK setup, repacking, re-testing, and uploading a new version, which is entirely unnecessary work. The worse cases is that the developer says "eh, screw this addon" and decides to stop maintaining it (which is a more likely case for an addon that has not been repacked since the SDK moved in-tree), complain somewhere on the internet, or both.

The case of "maybe my addon is actually fine despite what the validator says, I will test, bump compatibility, and harbor no resentment towards Mozilla" seems very unlikely to dominate developer response.

The key point here is that the issues here are entangling a much larger set of addon developers than usual (10 times as many, per comment 2). Some developers are probably accustomed to fixing up their addon often, but others (like myself) haven't touched their addon in six years. 

Addon developers are notoriously cranky and I'm sure you've developed an appropriately-thick skin for their criticism, but this unhappiness is actually going to be coming from a set of developers who have otherwise been happy for years. So unless we can establish that the case of SDK-only let bustage is small, I think this issue is very urgent and important.

Comment 9

[Andreas Wagner [:TheOne] [use NI]]

https://github.com/mozilla/amo-validator/pull/377

Comment 10

[Kris Maglione [:kmag]]

I'm inclined to agree. The compatibility bump emails don't earn us any goodwill as it is.

I was actually bribed with a bottle of whiskey to make them send one email per developer rather than per add-on. They happen too late in the release cycle for the warnings to be useful. They don't serve their original purpose, since the maxVersion flag is meaningless for most add-ons. But they still use the meaningless maxVersion flag to decide which add-ons to scan. The checks are almost always overly-broad regular expressions that trigger more false positives than actual positives. The warning text tends not to make it clear the warning very possibly a false positive. ...

When we send out an email to thousands of developers with a pretty dire-looking error that's actually meaningless, it just makes all of those problems much worse.

Comment 11

[Jorge Villalobos [:jorgev] (he/him)]

The patch is up and the new bulk validation was run. Here are the results:

Tested: 1547 (this is odd, since the previous run had 1010 failures, so those numbers should be closer)
Failed: 204
Passed: 	1337

So, there's definitely a big change and it's worth contacting devs about this again. Bug 1241154 is also possibly introducing noise to the compat validation failures, so I want to know first what its impact is before doing a second email blast. If the impact is large enough, I'll wait until that fix is checked in before doing so.

Comment 12

[Bobby Holley (:bholley)]

I personally think that a "hey sorry, the validator may have screwed up and we're working on it - sit tight" would be hugely valuable to do as soon as possible, in the event that it will take us until later than end-of-day to determine the exact extent of the false-positives.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Jorge Villalobos [:jorgev] from comment #11)
> The patch is up and the new bulk validation was run. Here are the results:
> 
> Tested: 1547 (this is odd, since the previous run had 1010 failures, so
> those numbers should be closer)

Does that mean that there are 1547 addons on AMO, or something else? If so, some of those addons may not be jetpack addons, and others may be jetpack addons without bundled SDKs (assuming that's what jetpack does now). Or am I misunderstanding?

Comment 14

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Bobby Holley (busy) from comment #13)
> Does that mean that there are 1547 addons on AMO, or something else? If so,
> some of those addons may not be jetpack addons, and others may be jetpack
> addons without bundled SDKs (assuming that's what jetpack does now). Or am I
> misunderstanding?

The bulk validator checks all add-ons that have a maxVersion within a certain range. For 44, it's checking for add-ons that have a maxVersion between 43.0a1 and 44.0. Those are the ones that end up in the Tested set. However, when the validation emails are sent, the add-ons that passed had their maxVersions bumped up to 44.*, so they aren't included in the second run.

My expectation is that the 1010 that failed during the first run would be pretty much the same add-ons that would be tested in the second run, but for some reason 1547 were tested, which is surprisingly large.

Comment 15

[Jorge Villalobos [:jorgev] (he/him)]

From bug 1241154, only the tests from 42 are being incorrectly included in the 44 run, so I think that's not worth waiting for. I'll finish the bulk validation and note the errors in the message.

Comment 16

[Jorge Villalobos [:jorgev] (he/him)]

The emails should have been sent out now.

Comment 17

[Bobby Holley (:bholley)]

Great!

For posterity, here's the email I just received:

> Dear add-on author,
>
> The previous compatibility validation run had some errors in it that caused many
> SDK-based add-ons to be incorrectly labeled as incompatible. This second run
> addresses this issue, so if your add-on passed this time around no further action
> is needed from you (though we always encourage you to test your add-ons in new
> versions of Firefox).
>
> We also discovered that some tests from the Firefox 42 compatibility run are showing
> up in this one. We couldn't fix this issue in time for this second run, therefore if
> you already addressed those errors in your code, you can safely change the maxVersion
> of your add-on to 44.* or higher so it is included in future compatibility tests.
>
> We apologize for the errors and extra messaging.
>
> --
>
>
> Our automated tests did not detect any compatibility issues with your add-on and
> Firefox 44.*. We've updated your add-on's compatibility to work with Firefox 44.* so
> that our beta and release users can begin using your add-on.
>
> We encourage you to view the results of the compatibility test, as some compatibility
> issues may have been detected but without enough certainty to declare the add-on incompatible:

Which sounds appropriate. Thanks for jumping on this!