Closed Bug 1302800 Opened 8 years ago Closed 8 years ago

Verify taskgraph implementations against documentation

Categories

(Firefox Build System :: Task Configuration, task)

Product:
Firefox Build System
Component:
Task Configuration
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla53

People

(Reporter: dustin, Assigned: hammad13060, Mentored)

References

Details

Attachments

(1 file)

Bug 1302800 - Verify taskgraph implementations against documentation;
58 bytes, text/x-review-board-request
dustin
: review+
Details 
The docs in taskcluster/docs contain a few lists of things -- attributes, parameters, kinds, caches, run-using implementations.

It would be great if something, either as part of the `mach taskgraph python-tests` or as part of the decision task, would ensure that those stay in sync with the code.

Some simple regexps over the documentation files would do the trick -- I don't think that trying to parse the files with docutils is a good use of time.
Assignee: nobody → hammad13060
Comment on attachment 8814487 [details]
Bug 1302800 - Verify taskgraph implementations against documentation;

I am trying to develop generic method for doc verification against implementation. 
Please provide your feedback.
P.S patch is incomplete right now
Comment on attachment 8814487 [details]
Bug 1302800 - Verify taskgraph implementations against documentation;

https://reviewboard.mozilla.org/r/95702/#review96026

::: taskcluster/taskgraph/generator.py:240
(Diff revision 2)
> +    @VerifyDoc.verify_doc_file("documentation and implementation for 'kinds' do not match", os.getcwd() + "/taskcluster/docs/")
> +    def verify_kinds(self, kinds):
> +        return {
> +            "kinds.rst": [kind for kind in kinds]
> +        }

I don't see a great advantage to using a decorator here.  Why not just

    def verify_kinds(self, kinds):
        verify_docs(
            filename='kinds.rst',
            identifiers=kinds.keys(),
            appearing_as='heading')

::: taskcluster/taskgraph/generator.py:256
(Diff revision 2)
> +            for attribute in task.attributes:
> +                attribute_set.add(attribute)

`attribute_set.update(task.attributes)` will be faster

::: taskcluster/taskgraph/generator.py:265
(Diff revision 2)
> +        for kind_label, kind in kinds.iteritems():
> +            if "jobs" in kind.config:
> +                for job_label, job in kind.config["jobs"].iteritems():
> +                    if "run" in job and "using" in job["run"]:
> +                        key = job["run"]["using"]
> +                        job_set.add(key)

This is reaching a little too deeply into the kind definitions.  In particular, they might have `jobs-from` instead of `jobs`.  Or some kinds might use completely different config.

However, `taskgraph.transforms.job.registry` has exactly the list of of run-using's that you need.

::: taskcluster/taskgraph/util/verifydoc.py:6
(Diff revision 2)
> +import re
> +import os
> +
> +class VerifyDoc(object):
> +
> +	@classmethod

A class with a single class method is just about always better implemented as a simple function.
Attachment #8814487 - Flags: review?(dustin)
I really like this comprehensive approach -- keep it up!
Hey Dustin thanks for the feedback :)
Can you elaborate on `taskgraph.transforms.job.registry`. Are you referring to registry dictionary in `transforms/__init__.py` ?
That's the one!
Hey Dustin, can you shed some light on caches ?
Comment on attachment 8814487 [details]
Bug 1302800 - Verify taskgraph implementations against documentation;

https://reviewboard.mozilla.org/r/95702/#review96888

I love the design!  This is great!

Just a few (ok, a lot) Python style points.  Some of these are probably applicable to any kind of Python, while others are specific to how we write Python in the Gecko source.

::: taskcluster/taskgraph/util/verifydoc.py:1
(Diff revision 7)
> +import re

This will need a copyright header like the other Python files (just copy/paste it)

Also, please use four-space indents.

::: taskcluster/taskgraph/util/verifydoc.py:4
(Diff revision 7)
> +import re
> +import os
> +
> +base_path = os.getcwd() + "/taskcluster/docs/"

best to use `os.path.join` here

::: taskcluster/taskgraph/util/verifydoc.py:8
(Diff revision 7)
> +
> +base_path = os.getcwd() + "/taskcluster/docs/"
> +
> +
> +def verify_docs(filename, identifiers, appearing_as):
> +	fileObject = open(base_path + filename)

In in-tree Python, we prefer `with open`:

    with open(os.path.join(base_path, filename)) as f:
        doctext = f.read()

::: taskcluster/taskgraph/util/verifydoc.py:10
(Diff revision 7)
> +	if appearing_as == "inline-literal":
> +		expression_list = ["``" + identifier + "``" for identifier in identifiers]
> +	elif appearing_as == "heading":
> +		expression_list = [identifier + "\n[-+\n*]+|[.+\n*]+" for identifier in identifiers]

Nicely done!

Please add an `else` branch here that raises an exception, to avoid typos like `"haeding"`.

::: taskcluster/taskgraph/util/verifydoc.py:15
(Diff revision 7)
> +	if appearing_as == "inline-literal":
> +		expression_list = ["``" + identifier + "``" for identifier in identifiers]
> +	elif appearing_as == "heading":
> +		expression_list = [identifier + "\n[-+\n*]+|[.+\n*]+" for identifier in identifiers]
> +
> +	for i in range(0, len(expression_list)):

`for expression, identifier in zip(expression_list, identifiers)`
Attachment #8814487 - Flags: review?(dustin)
Comment on attachment 8814487 [details]
Bug 1302800 - Verify taskgraph implementations against documentation;

https://reviewboard.mozilla.org/r/95702/#review97232

At a glance, this is looking better, but I see more review requests coming .. let me know when you're ready for me to do the final review!
Attachment #8814487 - Flags: review?(dustin)
(In reply to Dustin J. Mitchell [:dustin] from comment #19)
> Comment on attachment 8814487 [details]
> Bug 1302800 - Verify taskgraph implementations against documentation;
> 
> https://reviewboard.mozilla.org/r/95702/#review97232
> 
> At a glance, this is looking better, but I see more review requests coming
> .. let me know when you're ready for me to do the final review!

I am ready for final review :)
Comment on attachment 8814487 [details]
Bug 1302800 - Verify taskgraph implementations against documentation;

I am ready for final review :)
Comment on attachment 8814487 [details]
Bug 1302800 - Verify taskgraph implementations against documentation;

https://reviewboard.mozilla.org/r/95702/#review97638

::: taskcluster/docs/transforms.rst:154
(Diff revision 12)
> +  ``hazard``
> +  ``mach``
> +  ``mozharness``
> +  ``run-task``
> +  ``spidermonkey`` or ``spidermonkey-package`` or ``spidermonkey-mozjs-crate``
> +  ``toolchain-script``

I suspect this will render as a paragraph in the final output?  We'll probably want to make it a list.
Attachment #8814487 - Flags: review?(dustin) → review+
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/632a2fddbc63
Verify taskgraph implementations against documentation; r=dustin
https://hg.mozilla.org/mozilla-central/rev/632a2fddbc63
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Soooo, how do I run this test locally?

At first glance it looks to be part of the decision task alone, but if thats true, I think we should back out because a test that can break the entire build that can only be run in production (including try) isn't ideal for local iteration. (While the rest of the mach taskgraph stuff can indeed be run locally)

If it should be running as part of `./mach taskgraph full` it doesn't seem to be failing for me locally when I'm missing docs for some of the kinds on the `date` branch after merging this in...
Flags: needinfo?(hammad13060)
Comment on attachment 8814487 [details]
Bug 1302800 - Verify taskgraph implementations against documentation;

https://reviewboard.mozilla.org/r/95702/#review97700

::: taskcluster/taskgraph/util/verifydoc.py:9
(Diff revision 12)
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import re
> +import os
> +
> +base_path = os.path.join(os.getcwd(), "taskcluster/docs/")

Shouldn't this be using __file__ in some way, rather than relying on a known cwd that just-happens-to-match the topsrcdir
Comment on attachment 8814487 [details]
Bug 1302800 - Verify taskgraph implementations against documentation;

https://reviewboard.mozilla.org/r/95702/#review97704

::: taskcluster/taskgraph/util/verifydoc.py:18
(Diff revision 12)
> +    with open(os.path.join(base_path, filename)) as fileObject:
> +        doctext = "".join(fileObject.readlines())
> +        if appearing_as == "inline-literal":
> +            expression_list = ["``" + identifier + "``" for identifier in identifiers]
> +        elif appearing_as == "heading":
> +            expression_list = [identifier + "\n[-+\n*]+|[.+\n*]+" for identifier in identifiers]

Ah-Ha, found my issue with some debug logging...

 0:00.20 Testing match for expression u'balrog-l10n(?:\\n[-+\\n*]+|[\\.+\\n*]+)' using identifier u'balrog-l10n'
Traceback (most recent call last):
  File "/home/callek/mozilla/hg/mozilla-central/taskcluster/mach_commands.py", line 231, in show_taskgraph

That was after modifying this expression, the | here was effectively causing it to match either 'balrog-l10n\n[-+\n*]+' OR '[.+\n*]+'  I'll get a followup bug on file and a patch, unless you beat me to it.
Blocks: 1322193
Filed Bug 1322193  for the followup work, and assigned to Hammad for now.
Flags: needinfo?(hammad13060)
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: