User:Jorge.villalobos/Validator

Review of Basta's new validator

Setting up

• This took a while. I needed to upgrade my python install because it was 2.4.
• Used sudo pip-2.6 to install requirements.
• Still getting an error ImportError: No module named StringIO -> seems to require 2.6, doesn't work with 3.0 or later.
• Script is called addon-validator. The doc says package-parser.py.

Testing

• Tested an XML search add-on that our validator accepts. Got this in return:

   Summary:
  ------------------------------
  Detected type: Unknown
  ------------------------------
  Test failed! Errors:
  Error:	OpenSearch: The template for http://www.searchplugins.net/createos.aspx?number=45809:application/opensearchdescription+xml is missing
  Error:	The package is not of a recognized type.
  Error:	The XPI could not be opened.
  Extension Rejected

• • The type was 'unknown' even when I ran the validator with "-t search"
  • The first error seems to indicate the file type is in fact known but something weird happened while validating it. The URL returns a .NET server error... I have no idea what the given URL has to do with open search.
  • The following errors are odd. If the file is an XML, it doesn't make sense to even perform those checks.

• https://addons.mozilla.org/en-US/editors/review/116860
  • Current validator has one localization warning (not localized), which is true.
  • Basta's validator doesn't show any warnings.

• https://addons.mozilla.org/en-US/editors/review/117228
  • Basta's validator showed an error because of the maxVersion. They are hardcoded in targetapplication.py, and they should be using the DB instead.
  • After correcting this, Basta's validator doesn't show any warnings. There should be a localization warning and 2 setInterval warnings. Maybe Basta's validator has smarter checks for these?

• https://addons.mozilla.org/en-US/editors/review/117794
  • Current validator shows about 20 code warnings of different types.
  • Basta's validator only has this one, which hasn't made sense in a long time: Warning: Missing install.js for SeaMonkey.
  • Removing this check got me a "All tests succeeded".

• https://addons.mozilla.org/en-US/editors/review/117389
  • Again, Basta's validator failed to pick up on about 20 warnings.

In conclusion

Most of the important flags we use are either missing or there's a bug preventing them from appearing. I haven't analyzed the code yet to try to identify the cause. I'll do that next.

Basta's Comments

L10n

In terms of localization warnings/errors, the determination as to whether there is an error or not is based on statistics. Originally, I had programmed the validator to produce the exact output as the current validator. However, for many of the more popular add-ons (which are localized), warnings would be produced for untranslated entities. Most, if not all, of these entities were things like the add-on's name (AdBlock is still AdBlock in most languages, but it has different characters in some), "File", "Edit", etc. I added new criteria (minimum word length in order to count towards L10n completion, allowances between similar locales: en_US/en_GB/etc.), different threshold values, and other tweaks to help quiet the validator in many of the very load circumstances.

For just about every one of these tweaks, there should be a "constant" in the `/validator/testcases/l10ncompleteness.py` file. Overall, though, many of these changes to the validator have produced a generally positive outcome in terms of result quality.

http://github.com/mattbasta/amo-validator/blob/master/validator/testcases/l10ncompleteness.py#L21

XML Search Providers

In the case of the search provider error, there there may not necessarily be a bug in the validator. The validator does not intend for `<Url />` elements to have the value rel="self". In fact, the validator simply ignores that entirely. When I programmed the search provider validation feature, I based it entirely on the OpenSearch standard (http://www.opensearch.org/). The OS standard specifies that the {searchTerms} template parameter is required for a given template attribute of a URL element (though it could instead be present in one of the parameters, which the validator also handles).

In the case of the add-ons at http://searchplugins.net/, the <Url /> tag in question does not have the required {searchTerms} parameter (and has no <Param /> elements). As such, it is flagged as having an invalid template.

On the other hand, the spec does make reference to the rel="self" attribute, but does not describe any details about which parameters can/should be present. I'll tweak the validator to include checks for this element. While I'm in there, I'll also make the errors more clear and move the validation of the individual URLs to a higher point in the code that doesn't fully invalidate the XML's type.

maxVersion warning

The maxVersions were hardcoded because it was impractical to tie into the database for our purposes at the time. With Zamboni integration, this will be much more practical for both implementation as well as simplicity.

JS validation

As of now, the validator doesn't directly interface with Spidermonkey. One of my first goals will be to build this integration. With the validator as it stands, all JS blocks from all files are compiled into a "staging" directory. After moving this directory to the server, there is a simply Python script on the server which will invoke Spidermonkey and build out the trace for each of the files. The traces can then be downloaded and placed in another directory. The validator will look for the traces when the extension is re-validated.

As of now, however, the JS tests are rather weak (or non-existant, depending on which tests you are considering). These will be one of my top priorities.

Based on the code that I already have, however, the validator will be able to make some very complex conclusions about the code. For instance, if a dev assigns a variable (say, X) to contain a string, then accesses x["constructor"]["prototype"], the validator will keep track of all of that and know that the dev is trying to play with String.prototype. It will also be smart enough to evaluate literal expressions, and hopefully down the road, entire non-dynamic functions. Oh, and setTimeout will only be blocked if the first parameter is a string.

Other warnings/errors

I can't open the URLs that are listed, but I'd be more than happy to investigate what's causing the lack of warnings.