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

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Jonathan Griffin (:jgriffin)]

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.

Comment 1

[Jonathan Griffin (:jgriffin)]

Attachment: Run npm install as a separate step,

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.

Comment 2

[Jonathan Griffin (:jgriffin)]

I should probably also create a new error list for this.

Comment 3

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8366289 [details] [diff] [review]
Run npm install as a separate step,

LGTM :)

Comment 4

[Julien Wajsberg [:julienw]]

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.

Comment 5

[Jonathan Griffin (:jgriffin)]

(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.

Comment 6

[Gareth Aye [:gaye] (back from PTO)]

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.

Comment 7

[Julien Wajsberg [:julienw]]

(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.

Comment 8

[Jonathan Griffin (:jgriffin)]

(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.

Comment 9

[Jonathan Griffin (:jgriffin)]

Attachment: Run npm install as a separate step,

This is the current patch; I'll land it tomorrow morning.

Comment 10

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8367740 [details] [diff] [review]
Run npm install as a separate step,

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

Carry r+ forward.

Comment 11

[Julien Wajsberg [:julienw]]

(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 12

[Jonathan Griffin (:jgriffin)]

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

Comment 13

[Jonathan Griffin (:jgriffin)]

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

Comment 14

[Aki Sasaki (not active)]

In production.