Closed Bug 1336776 Opened 4 years ago Closed 4 years ago

Log friendlier error messages if MOZ_NODE_PATH environment variable is incorrect or not set

Categories

(Core :: Networking: HTTP, defect)

51 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- wontfix
firefox54 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 3 obsolete files)

I was trying to run xpcshell tests on a new machine and hadn't set the MOZ_NODE_PATH environment variable. I had to track back from an error message about an invalid port number in an in an http2 test. Since setting MOZ_NODE_PATH is required, we can log more useful error messages for the next person who tries to run xpcshell tests on a new machine. :)

ERROR Error: Invalid port in MOZHTTP2_PORT env var:  at obj-x86_64-apple-darwin16.4.0/_tests/xpcshell/dom/push/test/xpcshell/head-http2.js:7

Here is a green Try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b49c714052a062fa90aa0743d7895a50ab58315
Whiteboard: [necko-active]
Could you make all of these one commit? These commits all aimed at improving logging of failure conditions when trying to start node, and broken up like this, it's more difficult to see the change as a whole (which makes sense in this case). (Yes, I know mozreview might be able to show me what I want, but I'm also thinking of future code spelunking via vcs.) Thanks!
Flags: needinfo?(cpeterson)
(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #5)
> Could you make all of these one commit?

Sure. I'll post a squashed review request later today. I just thought it would be easier to review the smaller commits.
Flags: needinfo?(cpeterson)
Attachment #8833714 - Attachment is obsolete: true
Attachment #8833714 - Flags: review?(hurley)
Attachment #8833715 - Attachment is obsolete: true
Attachment #8833715 - Flags: review?(hurley)
Attachment #8833716 - Attachment is obsolete: true
Attachment #8833716 - Flags: review?(hurley)
Comment on attachment 8833713 [details]
Bug 1336776 - Log more useful error messages if MOZ_NODE_PATH or MOZHTTP2_PORT environment variables are not set correctly.

https://reviewboard.mozilla.org/r/109892/#review111790

This all looks pretty good to me. Just a couple nits on one line, and something to either do now or in a followup (if it's too complex).

Thanks!

::: testing/xpcshell/runxpcshelltests.py:997
(Diff revision 2)
>  
>          # We try to find the node executable in the path given to us by the user in
>          # the MOZ_NODE_PATH environment variable
> -        localPath = os.getenv('MOZ_NODE_PATH', None)
> -        if localPath and os.path.exists(localPath) and os.path.isfile(localPath):
> -            nodeBin = localPath
> +        nodeBin = os.getenv('MOZ_NODE_PATH', None)
> +        if not nodeBin:
> +            self.log.info('MOZ_NODE_PATH environment variable not set')

Add on to this something to the effect of "tests requiring http/2 will fail" as these checks don't prevent the tests from being run. I'm tempted to say you should make it an error, as well (since that will be easier to find when searching for why your tests failed).

::: testing/xpcshell/runxpcshelltests.py:1008
(Diff revision 2)
> -        elif nodeBin:
> -            self.log.info('Found node at %s' % (nodeBin,))
> +        self.log.info('Found node at %s' % (nodeBin,))
>  
> -            def startServer(name, serverJs):
> +        def startServer(name, serverJs):
> -                if os.path.exists(serverJs):
> +            if not os.path.exists(serverJs):
> +                self.log.error('%s not found at %s' % (name, serverJs))

Is there a way to make xpcshell tests hard fail? If we get to this point, there's no reason the file shouldn't exist unless your checkout or test package download is broken, so a hard fail seems most useful here.
Attachment #8833713 - Flags: review?(hurley) → review+
Comment on attachment 8833713 [details]
Bug 1336776 - Log more useful error messages if MOZ_NODE_PATH or MOZHTTP2_PORT environment variables are not set correctly.

https://reviewboard.mozilla.org/r/109892/#review111790

> Add on to this something to the effect of "tests requiring http/2 will fail" as these checks don't prevent the tests from being run. I'm tempted to say you should make it an error, as well (since that will be easier to find when searching for why your tests failed).

Oh, one other thing here - we should warn if MOZ_ASSUME_NODE_RUNNING is set, but MOZHTTP2_PORT isn't, since that will also cause failures.
(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #8)
> > +        if not nodeBin:
> > +            self.log.info('MOZ_NODE_PATH environment variable not set')
> 
> Add on to this something to the effect of "tests requiring http/2 will fail"
> as these checks don't prevent the tests from being run. I'm tempted to say
> you should make it an error, as well (since that will be easier to find when
> searching for why your tests failed).

I considered logging "MOZ_NODE_PATH not set" as an error, but most xpcshell tests don't require node, so logging this as an "error" seemed misleading. I can log it as a warning, however.


> > +            if not os.path.exists(serverJs):
> > +                self.log.error('%s not found at %s' % (name, serverJs))
> 
> Is there a way to make xpcshell tests hard fail? If we get to this point,
> there's no reason the file shouldn't exist unless your checkout or test
> package download is broken, so a hard fail seems most useful here.

We can raise a Python exception to abort the xpcshell tests.


(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #9)
> Oh, one other thing here - we should warn if MOZ_ASSUME_NODE_RUNNING is set,
> but MOZHTTP2_PORT isn't, since that will also cause failures.

OK. I'll update and resubmit my patch for your review with all these suggestions.
Comment on attachment 8833713 [details]
Bug 1336776 - Log more useful error messages if MOZ_NODE_PATH or MOZHTTP2_PORT environment variables are not set correctly.

I added a check for MOZHTTP2_PORT, log some messages as warnings (in yellow console text), and raise Python exceptions to abort the xpcshell tests if MOZ_NODE_PATH or serverJs are bad.
Attachment #8833713 - Flags: review+ → review?(hurley)
Comment on attachment 8833713 [details]
Bug 1336776 - Log more useful error messages if MOZ_NODE_PATH or MOZHTTP2_PORT environment variables are not set correctly.

https://reviewboard.mozilla.org/r/109892/#review112884

LGTM, thanks!
Attachment #8833713 - Flags: review?(hurley) → review+
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a71ca5d90c50
Log more useful error messages if MOZ_NODE_PATH or MOZHTTP2_PORT environment variables are not set correctly. r=nwgh
https://hg.mozilla.org/mozilla-central/rev/a71ca5d90c50
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.