Closed Bug 1225553 Opened 4 years ago Closed 4 years ago

Should validate that PERFHERDER_DATA works against the perfherder data schema in treeherder

Categories

(Testing :: Talos, defect)

defect
Not set

Tracking

(firefox45 fixed, b2g-v2.5 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: wlach, Assigned: parkouss)

References

Details

Attachments

(2 files)

In addition to validating that the PERFHERDER_DATA line is present for talos jobs, we should also double check that it validates against the perfherder schema.

This requires the perfherder schema file here, as well as the jsonschema python package:

https://github.com/mozilla/treeherder/blob/master/schemas/performance-artifact.json
https://pypi.python.org/pypi/jsonschema

Once you have those two things, you should be able to validation like this:

    import jsonschema
    perfherder_schema = json.load(open(perfherder_schema_file))
    jsonschema.validate(perfherder_schema, perfherder_blob)

The check should probably be somewhere around here: http://mxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/talos.py#372
Sounds good! So I believe you are talking about the jsonschema available on pypi:

https://pypi.python.org/packages/py2.py3/j/jsonschema/jsonschema-2.5.1-py2.py3-none-any.whl#md5=20a47c9d9bc9357d8c731cfc19e3f968

For python 2.7, it requires functools32:

https://pypi.python.org/packages/source/f/functools32/functools32-3.2.3-2.tar.gz#md5=09f24ffd9af9f6cd0f63cb9f4e23d4b2

So I guess we will need to add those in internal pypi [1], since it has to be used from mozharness runs.

Speaking about code quality checks (kind of), I would like to reopen the flake8 subject. We could easily add a flake8 run in mozharness (talos) also, to ensure some basic stuff like unused imports/variables, non defined variables and so on. I would go for a default configuration for the tool (meaning 79 chars max line length for example) but that can be adjusted ofc.

In case you like the idea, I would be happy to work on that! And in that case, we would need in internal pypi the following packages:

https://pypi.python.org/packages/py2.py3/f/flake8/flake8-2.5.0-py2.py3-none-any.whl#md5=dfdca9dff574ee0d1d3af2b6c84487ae
https://pypi.python.org/packages/py2.py3/m/mccabe/mccabe-0.3.1-py2.py3-none-any.whl#md5=bea6b1fdd1cffb8afb4d7cac046f104f
https://pypi.python.org/packages/py2.py3/p/pep8/pep8-1.6.2-py2.py3-none-any.whl#md5=d1c1f046a98f165628f60106faaee35a
https://pypi.python.org/packages/2.7/p/pyflakes/pyflakes-1.0.0-py2.py3-none-any.whl#md5=b06f8034c94c72bcdc4da29a4753b8b7

We can probably ask for putting all them in internal pypi in one bug.

[1] http://pypi.pub.build.mozilla.org/pub/
(In reply to William Lachance (:wlach) from comment #0)
> This requires the perfherder schema file here, as well as the jsonschema
> python package:
> 
> https://github.com/mozilla/treeherder/blob/master/schemas/performance-
> artifact.json
> https://pypi.python.org/pypi/jsonschema

Oh, yeah, you *are* talking about the jsonschema on pypi. Sorry, I missed that. :)

As I never remember how to open bugs for internal pypi mirroring, here is a similar bug that we can take as an example:

https://bugzilla.mozilla.org/show_bug.cgi?id=1189720

Waiting for your answers about flake8, then I'll be glad to open the bug and work on this.
I would like to see flake8 run on the talos code, specifically in automation.  

A nice larger project would be to have a specific job to run test harness unit tests (i.e. not at build time) and other tools like pyflakes, flake8, etc.

that might be too much work for now, I suspect adding <5 seconds of overhead per talos job by adding flake8 into mozharness would be a good middle of the road approach.
Flags: needinfo?(wlachance)
(In reply to Joel Maher (:jmaher) from comment #3)
> I would like to see flake8 run on the talos code, specifically in
> automation.  
> 
> A nice larger project would be to have a specific job to run test harness
> unit tests (i.e. not at build time) and other tools like pyflakes, flake8,
> etc.
> 
> that might be too much work for now, I suspect adding <5 seconds of overhead
> per talos job by adding flake8 into mozharness would be a good middle of the
> road approach.

I think it makes more sense to run the check at build time, to catch problems sooner. Just like we do with the mozbase tests, for example. Running flake8 really doesn't take much time.

We should discuss this in a seperate bug though. I'd like to keep this one focused on making sure PERFHERDER_OUTPUT is correct.
Flags: needinfo?(wlachance)
(In reply to William Lachance (:wlach) from comment #4)
> I think it makes more sense to run the check at build time, to catch
> problems sooner. Just like we do with the mozbase tests, for example.
> Running flake8 really doesn't take much time.
> 
> We should discuss this in a seperate bug though. I'd like to keep this one
> focused on making sure PERFHERDER_OUTPUT is correct.

Awesome, let's do that! I will just open one bug to copy all the dependencies in internal pypi in once, and make this bug and a new one for flake8 (or reuse one, maybe there is one already) depend on it.
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
Depends on: 1226841
Bug 1225553 - Should validate that PERFHERDER_DATA works against the perfherder data schema in treeherder. r=jmaher
Attachment #8690393 - Flags: review?(jmaher)
Attachment #8690393 - Flags: feedback?(wlachance)
This show the output when validation fail. I broken talos on purpose, changing the key 'name' to 'name123'.

So locally it works well. We'll see on try once pypi dependencies will be available.
Comment on attachment 8690393 [details]
MozReview Request: Bug 1225553 - Should validate that PERFHERDER_DATA works against the perfherder data schema in treeherder. r=jmaher

https://reviewboard.mozilla.org/r/25853/#review23247

I like this and appreciate the comments to explain the odd thinks (like the late import)
Attachment #8690393 - Flags: review?(jmaher) → review+
Ok, everything good now (I rerun the builds on the try push)! Waiting for Will's feedback.
https://reviewboard.mozilla.org/r/25853/#review23357

::: testing/mozharness/mozharness/mozilla/testing/talos.py:343
(Diff revision 1)
> +            self.exception("Error while validating PERFHERDER_DATA")

Is there a way we could more explicitly show the error? I believe jsonschema emits detailed error messages showing the exact origin of the problem.

This looks great, just one request for improvement.
Attachment #8690393 - Flags: feedback?(wlachance) → feedback+
https://reviewboard.mozilla.org/r/25853/#review23357

> Is there a way we could more explicitly show the error? I believe jsonschema emits detailed error messages showing the exact origin of the problem.

It is shown - the exception function does it (look at my attachment on the bug)
https://hg.mozilla.org/integration/mozilla-inbound/rev/50785278334f99a09ba70d31f760fcf83fcbb764
Bug 1225553 - Should validate that PERFHERDER_DATA works against the perfherder data schema in treeherder. r=jmaher
https://hg.mozilla.org/mozilla-central/rev/50785278334f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.