Tracking bug for Jetpack SDK code reviews

RESOLVED FIXED in 0.5

Status

Add-on SDK
General
P1
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: adw, Assigned: dietrich)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

8 years ago
As discussed in today's meeting, the code in the SDK 0.1 release hasn't been reviewed, but it needs to be.  We'll use this bug to track that progress.  Make a bug for each review and mark it blocking this one.
(Reporter)

Updated

8 years ago
Depends on: 551346
(Reporter)

Updated

8 years ago
Depends on: 551348
(Reporter)

Updated

8 years ago
Depends on: 551349
(Reporter)

Updated

8 years ago
Depends on: 551352
(Reporter)

Updated

8 years ago
Depends on: 551353
(Reporter)

Updated

8 years ago
Depends on: 551354
(Reporter)

Updated

8 years ago
Depends on: 551355
(Reporter)

Updated

8 years ago
Depends on: 551356
(Reporter)

Updated

8 years ago
Depends on: 551357
(Reporter)

Updated

8 years ago
Depends on: 551358
(Reporter)

Updated

8 years ago
Depends on: 551360
(Reporter)

Updated

8 years ago
Depends on: 551361
(Reporter)

Updated

8 years ago
Depends on: 551362
(Reporter)

Updated

8 years ago
Depends on: 551364
(Reporter)

Updated

8 years ago
Depends on: 551365
Awesome, thanks for filing these Drew!

A couple notes...

We absolutely need to do this, and we shouldn't delay it too long. However, it's not the top priority for the 0.2 cycle, where we should make sure we continue to focus on standing up the initial high-level APIs we've been specing.

Folks who aren't working on 0.2 priorities, however, are welcome to tackle these reviews immediately!

I also want to make sure that all the code we ask folks to review is really on the production train.

Atul: is there anything in the SDK that you consider to still be prototype/experimental code and which you intended to replace with production-quality code at a later date?  If so, we should delay reviews of that code until its replacement is in place (and file bugs on implementing those replacements).

Comment 2

8 years ago
Myk, there isn't anything that's prototype/experimental code, but I should mention that when I wrote the SDK, I didn't actually write any tests for any code that wouldn't run on end-user systems.

That means, for instance, that the cfx tool doesn't have any tests, and generally isn't of the same quality as the code in the 'jetpack-core' package that Drew has made bugs for. For that matter, nor does the actual test harness/runner program, contained in the 'test-harness' package.

As hanson mentioned in the Labs meeting yesterday, part of the reason for code review is to improve the "bus factor" for code by ensuring that more folks know how it works, so it might be useful to review the cfx tool and the test-harness package--besides, it would be terrible if our tests were failing but looked like they passed because of a bug in the test harness program.

I've just sent out an email to Ian Bicking and Brian Warner asking them if they'd be interested in either reviewing or writing tests for the cfx tool.

Also, it's imperative that someone review the file at python-lib/cuddlefish/app-extension/components/harness.js, as this is the "bootstrapper" called by Firefox which creates an instance of the Jetpack Runtime and starts the Jetpack-based extension.  I should actually probably document this a bit more before someone does this, though, because I suspect code reviewing it without some docs might be fairly painful.

Comment 3

8 years ago
Also, while none of the code is prototype code, I think there are some "TODOs" in the jetpack-core package that we may want to make bugs for. A grep for the string "TODO" should do the trick.
We still need to come up with a plan for when we're going to do these reviews.  In the meantime, removing the "0.1" piece from the summary, as some of the code we're going to review has landed since 0.1.
Summary: Tracking bug for Jetpack SDK 0.1 code reviews → Tracking bug for Jetpack SDK code reviews
(Reporter)

Comment 5

8 years ago
Ah, sorry, I meant to ping Dietrich about this again.  Dietrich, would it be OK to roll this up into the JEP implementation goal for fx team?
(Assignee)

Comment 6

8 years ago
Not sure it makes sense to tie it to our JEP implementation plan, but I'm definitely up for helping find the right reviewers here. I'll spend some time looking at it this week.
(Assignee)

Updated

8 years ago
Assignee: nobody → dietrich
Priority: -- → P1
Target Milestone: -- → 0.3
We're not going to finish all these reviews for 0.3, nor do I think it's necessary.  I would, however, like us to have these done by the time we release 0.5 in late June, such that all the code in the 0.5 release is reviewed.
Target Milestone: 0.3 → 0.5
(Assignee)

Comment 8

8 years ago
Sounds good. Given how the reviews have gone so far, they definitely seem to have value. And will take a while.
(Assignee)

Updated

8 years ago
Target Milestone: 0.5 → 0.3
(Assignee)

Updated

8 years ago
Target Milestone: 0.3 → 0.5
The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
Tracking bug no longer needed.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
No longer depends on: 551346
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.