Closed Bug 964527 Opened 8 years ago Closed 8 years ago

Run 'npm install' as a separate step in the gaia-integration tests

Categories

(Release Engineering :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jgriffin, Assigned: jgriffin)

References

Details

Attachments

(1 file, 1 obsolete file)

We should run 'npm install' as a separate step prior to 'make test-integration' when the integration tests are run on TBPL, so we can handle failures during 'npm install' in a particular fashion.
The intent of this patch is to run 'npm install' separately from 'make test-integration'.  If it fails, it will be retried up to 3 times, and if it fails all 3 times, we'll bail with a distinct error message that will make it more distinguishable from a generic timeout error.

I'm going to test this on ash.
Attachment #8366289 - Flags: review?(gaye)
I should probably also create a new error list for this.
Comment on attachment 8366289 [details] [diff] [review]
Run npm install as a separate step,

LGTM :)
Attachment #8366289 - Flags: review?(gaye) → review+
Just a thought, maybe it's better to run "make node_modules/.bin/mozilla-download" so that you're sure that you're running the same command than the Makefile. Not sure that matters though.
(In reply to Julien Wajsberg [:julienw] from comment #4)
> Just a thought, maybe it's better to run "make
> node_modules/.bin/mozilla-download" so that you're sure that you're running
> the same command than the Makefile. Not sure that matters though.

Isn't mozilla-download used to download a b2g desktop build?  If so, that's not what we want to do here, since we want to use the build that triggered the job in TBPL, and not just the most recent build.
Usually the npm install happens in ./bin/gaia-marionette, but in this case we'd like to isolate it from the rest of the stuff that happens so that we can retry in the case of transient failures and also have some more specific logging.
(In reply to Jonathan Griffin (:jgriffin) from comment #5)
> (In reply to Julien Wajsberg [:julienw] from comment #4)
> > Just a thought, maybe it's better to run "make
> > node_modules/.bin/mozilla-download" so that you're sure that you're running
> > the same command than the Makefile. Not sure that matters though.
> 
> Isn't mozilla-download used to download a b2g desktop build?  If so, that's
> not what we want to do here, since we want to use the build that triggered
> the job in TBPL, and not just the most recent build.

My suggestion was because this would trigger this Makefile goal:

https://github.com/mozilla-b2g/gaia/blob/master/Makefile#L681-L684

We can add a PHONY "npm-install" goal on the same line if that makes things saner.
(In reply to Julien Wajsberg [:julienw] from comment #7)
> (In reply to Jonathan Griffin (:jgriffin) from comment #5)
> > (In reply to Julien Wajsberg [:julienw] from comment #4)
> > > Just a thought, maybe it's better to run "make
> > > node_modules/.bin/mozilla-download" so that you're sure that you're running
> > > the same command than the Makefile. Not sure that matters though.
> > 
> > Isn't mozilla-download used to download a b2g desktop build?  If so, that's
> > not what we want to do here, since we want to use the build that triggered
> > the job in TBPL, and not just the most recent build.
> 
> My suggestion was because this would trigger this Makefile goal:
> 
> https://github.com/mozilla-b2g/gaia/blob/master/Makefile#L681-L684
> 
> We can add a PHONY "npm-install" goal on the same line if that makes things
> saner.

Ah, I see.  The only disadvantage of doing things this way is that we lose the output of 'npm install' (which doesn't get echoed to stdout), and sometimes this might be useful for debugging.  If we modified the makefile to optionally echo this, we could switch to doing that.
This is the current patch; I'll land it tomorrow morning.
Attachment #8366289 - Attachment is obsolete: true
Comment on attachment 8367740 [details] [diff] [review]
Run npm install as a separate step,

Review of attachment 8367740 [details] [diff] [review]:
-----------------------------------------------------------------

Carry r+ forward.
Attachment #8367740 - Flags: review+
(In reply to Jonathan Griffin (:jgriffin) from comment #8)
> (In reply to Julien Wajsberg [:julienw] from comment #7)
> > (In reply to Jonathan Griffin (:jgriffin) from comment #5)
> > > (In reply to Julien Wajsberg [:julienw] from comment #4)
> > > > Just a thought, maybe it's better to run "make
> > > > node_modules/.bin/mozilla-download" so that you're sure that you're running
> > > > the same command than the Makefile. Not sure that matters though.
> > > 
> > > Isn't mozilla-download used to download a b2g desktop build?  If so, that's
> > > not what we want to do here, since we want to use the build that triggered
> > > the job in TBPL, and not just the most recent build.
> > 
> > My suggestion was because this would trigger this Makefile goal:
> > 
> > https://github.com/mozilla-b2g/gaia/blob/master/Makefile#L681-L684
> > 
> > We can add a PHONY "npm-install" goal on the same line if that makes things
> > saner.
> 
> Ah, I see.  The only disadvantage of doing things this way is that we lose
> the output of 'npm install' (which doesn't get echoed to stdout), and
> sometimes this might be useful for debugging.  If we modified the makefile
> to optionally echo this, we could switch to doing that.

I'm surprised, we don't do redirect stdout so we should get the output just fine.
Comment on attachment 8367740 [details] [diff] [review]
Run npm install as a separate step,

Review of attachment 8367740 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/build/mozharness/rev/f466be651afa
Attachment #8367740 - Flags: checked-in+
This patch is working well on cedar; here's a log in which 'npm install' was retried after initially failing, and this time it passed:

https://tbpl.mozilla.org/php/getParsedLog.php?id=33823435&tree=Cedar&full=1
In production.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 966156
Depends on: 966165
Depends on: 966293
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.