Closed Bug 1425708 Opened 6 years ago Closed 6 years ago

Document advice for working on Marionette source code

Categories

(Remote Protocol :: Marionette, enhancement)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1425708 - Document advice for working on Marionette.
59 bytes, text/x-review-board-request
automatedtester
: review+
impossibus
: review+
Details
Bug 1425708 - Link to contribution and source docs in Marionette README.
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1425708 - Simplify and make contribution advice coherent.
59 bytes, text/x-review-board-request
impossibus
: review+
Details 
There are many gotchas to be aware of when working on Marionette
source code.  It would be a good idea to collect this in a single
document that could serve as documentation in onboarding new
developers.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment on attachment 8937305 [details]
Bug 1425708 - Document advice for working on Marionette.

https://reviewboard.mozilla.org/r/208014/#review213896

Nice!
I made a bunch of suggestions -- go with whatever you feel is best.

::: testing/marionette/CONTRIBUTING.md:51
(Diff revision 1)
>  
>  The canonical source code repository is [mozilla-central].  Bugs are
>  filed in the [`Testing :: Marionette`] component on Bugzilla.  We also
>  have a curated set of [good first bugs] you may consider attempting first.
>  
> -The purpose of this guide _is not_ to make sure you have a basic
> +The purpose of this guide is not to make sure you have a basic

I would move this paragraph to the top of the _Building_ section. The Developer Guide has a tonne of (conflicting) information in it, so I think it's better that people see that link in the context of the simplified build instructions we provide.

::: testing/marionette/CONTRIBUTING.md:57
(Diff revision 1)
>  development environment set up.  For that there is plentiful
>  documentation, such as the [Developer Guide] to get you rolling.
>  Once you do, we can get started working up your first patch!
>  Remember to [reach out to us] at any point if you have questions.
>  
> +We have collected a lot of good advice for working on Marionette

I wonder, maybe we should move README.md and CONTRIBUTING.md into testing/marionette/docs (or at least include links in the TOC) so that all the documentation is super discoverable. Similarly, link to Firefox Source Docs in the README.md

::: testing/marionette/CONTRIBUTING.md:71
(Diff revision 1)
>  
>  Building
>  --------
>  
>  As Marionette is built in to Firefox and ships with official Firefox
>  releases, it is included in a normal Firefox build.  To get your

Insert: Once you have a clone of mozilla-unified [you can run ./mach bootstrap etc]

::: testing/marionette/CONTRIBUTING.md:81
(Diff revision 1)
>  
>  As Marionette is written in [XPCOM] flavoured JavaScript, you may
>  choose to rely on so called [artifact builds], which will download
>  pre-compiled Firefox blobs to your computer.  This means you don’t
>  have to compile Firefox locally, but does come at the cost of having
>  a good internet connection.  To enable [artifact builds] you may

Simpler: To enable artifact builds, chose "Firefox for Desktop Artifact Mode" when you run `./mach bootstrap`

::: testing/marionette/doc/CodeStyle.md:248
(Diff revision 1)
> +The linter is also run as a try job which means any style violations
> +will automatically block a patch from landing (if using Autoland)
> +or cause your changeset to be backed out (if landing directly on
> +mozilla-inbound).
> +
> +If you use git(1) you can enable automatic linting before you push

Just link to https://firefox-source-docs.mozilla.org/tools/lint/usage.html#using-a-vcs-hook instead of documenting hooks here?

::: testing/marionette/doc/CodeStyle.md:261
(Diff revision 1)
> +	% ln -s tools/lint/hooks.py .git/hooks/pre-commit
> +
> +The next time you `git commit` the linter will run automatically
> +and save you from frustration and wasting precious try resources.
> +
> +[mozlint]: https://firefox-source-docs.mozilla.org/python/mozlint.html

I think this is a more user-friendly link: https://firefox-source-docs.mozilla.org/tools/lint/usage.html
Attachment #8937305 - Flags: review?(mjzffr) → review+
Comment on attachment 8937305 [details]
Bug 1425708 - Document advice for working on Marionette.

https://reviewboard.mozilla.org/r/208014/#review213896

Thanks a lot for reading through it!

I think I’ve made every change you suggested, except moving
README.md and CONTRIBUTING.md under doc/.  Read more below.

> I would move this paragraph to the top of the _Building_ section. The Developer Guide has a tonne of (conflicting) information in it, so I think it's better that people see that link in the context of the simplified build instructions we provide.

Actually I don’t find this paragraph very valuable at all.  All the
steps you need are documented here.  As you say, the Developer
Guide also has a lot of conflicting stuff in it, so maybe we are
better off shaving away this bit.

> I wonder, maybe we should move README.md and CONTRIBUTING.md into testing/marionette/docs (or at least include links in the TOC) so that all the documentation is super discoverable. Similarly, link to Firefox Source Docs in the README.md

I see why you want to do this, but it is customary to have
these files in the top-level directory of a project, which
is where I would look for them.  A secondary benefit is
that GitHub automatically picks up the files when visiting
https://github.com/mozilla/gecko/tree/central/testing/marionette.

Is there somehow we can link/include them in the source code if we
don’t want to move them under doc/?
Comment on attachment 8937478 [details]
Bug 1425708 - Simplify and make contribution advice coherent.

https://reviewboard.mozilla.org/r/208150/#review213910

::: testing/marionette/CONTRIBUTING.md:72
(Diff revision 1)
>  As Marionette is written in [XPCOM] flavoured JavaScript, you may
>  choose to rely on so called [artifact builds], which will download
>  pre-compiled Firefox blobs to your computer.  This means you don’t
>  have to compile Firefox locally, but does come at the cost of having
>  a good internet connection.  To enable [artifact builds] you may
> -add this line to the _mozconfig_ file in your top source directory:
> +choose ‘Firefox for Desktop Artifact Mode’ when bootstrapping.

Maybe this paragraph should come first?  Whenever I read documentation
I run the command when the documentation says I should.  For the
following paragraph to then say I should’ve done something
different is maybe a little bit confusing?
Comment on attachment 8937477 [details]
Bug 1425708 - Link to contribution and source docs in Marionette README.

https://reviewboard.mozilla.org/r/208148/#review213970

::: testing/marionette/README.md:49
(Diff revision 1)
> -Bugs
> -====
> +Contributing
> +============
>  
> -Bugs are tracked in the `Testing :: Marionette` component.
> +If you want to help improve Marionette, we have more information in
> +<CONTRIBUTING.md>.  You will find additional development documentation at
> +<https://firefox-source-docs.mozilla.org/testing/marionette/marionette>.

Any idea why we have `marionette` twice in the URL? No other section has that.
Comment on attachment 8937477 [details]
Bug 1425708 - Link to contribution and source docs in Marionette README.

https://reviewboard.mozilla.org/r/208148/#review213970

> Any idea why we have `marionette` twice in the URL? No other section has that.

I didn’t spot that, but I don’t know the answer to your question.
Maybe maja_zf will?
Comment on attachment 8937305 [details]
Bug 1425708 - Document advice for working on Marionette.

https://reviewboard.mozilla.org/r/208014/#review213978

::: testing/marionette/doc/CodeStyle.md:71
(Diff revision 3)
> +omitted:
> +
> +	const {TYPE_ONE_SHOT, TYPE_REPEATING_SLACK} = Ci.nsITimer;
> +
> +In addition to the default [Mozilla eslint rules], we have our
> +own specialisations that are stricter and enforce more security.

Lets link to the linter config we own. And maybe we can document those restrictions over there, and not in this document? Changing one setting would cause us to also update this document, and I'm sure we will forget about it.

::: testing/marionette/doc/CodeStyle.md:98
(Diff revision 3)
> +and complain about unused variables.
> +
> +For purely aesthetic reasons we indent our code with two spaces,
> +which includes switch-statement `case`s, and limit the maximum
> +line length to 78 columns.  This is not normally something you have
> +to think to deeply about as it is enforced by the [linter].

Please don't miss the 4-char indentation you want to see for wrapped lines.

::: testing/marionette/doc/CodeStyle.md:140
(Diff revision 3)
> +	  let name = msg.name;
> +	  let target = msg.target;
> +	  let json = msg.json;
> +	  let data = msg.data;
> +	  …
> +	};

You know that I'm not a fan of this. My preferred way is still:

```
const responseListener = msg => { 
  let {name, target, json, data} = msg;
  …
};
```

Nothing is more irritating as a non-readable function argument list.

::: testing/marionette/doc/CodeStyle.md:241
(Diff revision 3)
> +different errors:
> +
> +	% ./mach lint -funix testing/marionette | awk -F: '{ print $1, $4 }' | uniq -c | sort
> +	      4 testing/marionette/driver.js  semi error
> +	      2 testing/marionette/driver.js  comma-dangle error
> +	      1 testing/marionette/listener.js  semi error

Personally I find this paragraph tells too much detail no-one should be confronted with. I also never had to use that myself yet. So I doubt we need those lines listed here.

::: testing/marionette/doc/CodeStyle.md:246
(Diff revision 3)
> +	      1 testing/marionette/listener.js  semi error
> +
> +The linter is also run as a try job which means any style violations
> +will automatically block a patch from landing (if using Autoland)
> +or cause your changeset to be backed out (if landing directly on
> +mozilla-inbound).

Add the job name of the try job here, so people can have a look at Treeherder themselves.

::: testing/marionette/doc/CodeStyle.md:248
(Diff revision 3)
> +The linter is also run as a try job which means any style violations
> +will automatically block a patch from landing (if using Autoland)
> +or cause your changeset to be backed out (if landing directly on
> +mozilla-inbound).
> +
> +If you use git(1) you can [enable automatic linting] before you push

This doesn't only work for git. Also when using `hg` you can enable that. I'm using a precommit hook and not a prepush hook btw.
Attachment #8937305 - Flags: review?(hskupin)
Comment on attachment 8937477 [details]
Bug 1425708 - Link to contribution and source docs in Marionette README.

https://reviewboard.mozilla.org/r/208148/#review213970

> I didn’t spot that, but I don’t know the answer to your question.
> Maybe maja_zf will?

I did it intentionally to annoy you both. :P

Nothing obvious in my original setup indicates a double-marionette, unless I'm blind: https://hg.mozilla.org/mozilla-central/rev/84139047f8dc

I see that the mozbase moz.build file has `SPHINX_TREES['/mozbase'] = 'docs'` (note the slash), so maybe '/marionette' in our moz.build will fix things.
Comment on attachment 8937477 [details]
Bug 1425708 - Link to contribution and source docs in Marionette README.

https://reviewboard.mozilla.org/r/208148/#review213984
Attachment #8937477 - Flags: review?(mjzffr) → review+
Comment on attachment 8937478 [details]
Bug 1425708 - Simplify and make contribution advice coherent.

https://reviewboard.mozilla.org/r/208150/#review213910

> Maybe this paragraph should come first?  Whenever I read documentation
> I run the command when the documentation says I should.  For the
> following paragraph to then say I should’ve done something
> different is maybe a little bit confusing?

I agree, do iiiit.
Comment on attachment 8937478 [details]
Bug 1425708 - Simplify and make contribution advice coherent.

https://reviewboard.mozilla.org/r/208150/#review213986

::: testing/marionette/CONTRIBUTING.md:54
(Diff revision 1)
>  have a curated set of [good first bugs] you may consider attempting first.
>  
> -The purpose of this guide _is not_ to make sure you have a basic
> -development environment set up.  For that there is plentiful
> -documentation, such as the [Developer Guide] to get you rolling.
> -Once you do, we can get started working up your first patch!
> +We have collected a lot of good advice for working on Marionette
> +code in <doc/CodeStyle.md>, which we highly recommend you read.
> +There is more development documentation archived under <doc/index.rst>
> +that you may preuse at your convenience.

s/preuse/peruse/
Attachment #8937478 - Flags: review?(mjzffr) → review+
Comment on attachment 8937305 [details]
Bug 1425708 - Document advice for working on Marionette.

https://reviewboard.mozilla.org/r/208014/#review213896

> I see why you want to do this, but it is customary to have
> these files in the top-level directory of a project, which
> is where I would look for them.  A secondary benefit is
> that GitHub automatically picks up the files when visiting
> https://github.com/mozilla/gecko/tree/central/testing/marionette.
> 
> Is there somehow we can link/include them in the source code if we
> don’t want to move them under doc/?

I would just link to https://searchfox.org/mozilla-central/source/testing/marionette/README.md etc.
Blocks: 1426133
Blocks: 1426136
Comment on attachment 8937305 [details]
Bug 1425708 - Document advice for working on Marionette.

https://reviewboard.mozilla.org/r/208014/#review213978

Thanks for the feedback.

> Lets link to the linter config we own. And maybe we can document those restrictions over there, and not in this document? Changing one setting would cause us to also update this document, and I'm sure we will forget about it.

I can link to the configuration but I fear it doesn’t serve well
as self-documentation because it is coalesced with other eslint
configurations in the tree.

> Please don't miss the 4-char indentation you want to see for wrapped lines.

It’s clear you don’t like this, do you have a suggestion how to
distinguish wrapped lines from a new block indentation?

This issue also reminded me to document "./mach eslint --fix".

> You know that I'm not a fan of this. My preferred way is still:
> 
> ```
> const responseListener = msg => { 
>   let {name, target, json, data} = msg;
>   …
> };
> ```
> 
> Nothing is more irritating as a non-readable function argument list.

So your preferred way is basically the same, except msg is repeated
instead of expanded directly.

Judging by what I can find in m-c [1] it looks like the example on
:130 is widespread in Gecko code.  It’s harder to search for the
opposite, but it looks to me like the "foo => …" form is more popular
for non-object values (callables, primitives).  Can you find any
evidence to the contrary?

  [1] https://searchfox.org/mozilla-central/search?q=%7D)+%3D%3E&case=false&regexp=false&path=
  [2] https://searchfox.org/mozilla-central/search?q=(%5Ba-z%5D%2B)+%3D%3E&case=false&regexp=true&path=

> Personally I find this paragraph tells too much detail no-one should be confronted with. I also never had to use that myself yet. So I doubt we need those lines listed here.

I thought I’d document it here as a clever usage of the output.  I
have used it on occasion for larger refactorings, but I can remove
it if you find it more distracting than useful.

> Add the job name of the try job here, so people can have a look at Treeherder themselves.

Good idea.

Your issue also reminds me that we should document what try jobs
Marionette is used in, and also that we should populate the
IMPACTED_TESTS variable in testing/marionette/moz.build.  Filed [1]
and [2] about this.

  [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1426136
  [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1426133
Comment on attachment 8937305 [details]
Bug 1425708 - Document advice for working on Marionette.

https://reviewboard.mozilla.org/r/208014/#review214578

::: testing/marionette/doc/CodeStyle.md:156
(Diff revision 4)
> +so that the file is parsed in [strict mode].
> +
> +Every source code file that ships as part of the Firefox bundle
> +must also have the following copying header:
> +
> +	/* This Source Code Form is subject to the terms of the Mozilla Public

Instead of having this hear it would be better to link out to the header at https://www.mozilla.org/en-US/MPL/headers/ in case policy changes the wording (though unlikely)
Attachment #8937305 - Flags: review?(dburns) → review+
Thanks for all the feedback, everyone!  I believe I have made
sufficient changes based on your input that this can land in the tree now.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1ac8945353a
Link to contribution and source docs in Marionette README. r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4c64270c97
Simplify and make contribution advice coherent. r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/00be73b22814
Document advice for working on Marionette. r=maja_zf,automatedtester
https://hg.mozilla.org/integration/mozilla-inbound/rev/2adec22ad184
Link to Marionette README. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/b48baef2477f
Fix link to contribution advice. r=me
Comment on attachment 8937305 [details]
Bug 1425708 - Document advice for working on Marionette.

https://reviewboard.mozilla.org/r/208014/#review213978

> It’s clear you don’t like this, do you have a suggestion how to
> distinguish wrapped lines from a new block indentation?
> 
> This issue also reminded me to document "./mach eslint --fix".

I haven't said that I don't like it per se, it's just not something which is used by the generial Mozilla coding style rules for Javascript. It's something special only for Marionette.

> So your preferred way is basically the same, except msg is repeated
> instead of expanded directly.
> 
> Judging by what I can find in m-c [1] it looks like the example on
> :130 is widespread in Gecko code.  It’s harder to search for the
> opposite, but it looks to me like the "foo => …" form is more popular
> for non-object values (callables, primitives).  Can you find any
> evidence to the contrary?
> 
>   [1] https://searchfox.org/mozilla-central/search?q=%7D)+%3D%3E&case=false&regexp=false&path=
>   [2] https://searchfox.org/mozilla-central/search?q=(%5Ba-z%5D%2B)+%3D%3E&case=false&regexp=true&path=

All the above examples are on a single line, and don't span multiple lines, as what we currently have in multiple cases in our code, which IMHO makes code harder to read. You might feel different.
Comment on attachment 8937305 [details]
Bug 1425708 - Document advice for working on Marionette.

Removing review request given that this patch got already landed.
Attachment #8937305 - Flags: review?(hskupin)
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: