837897 - test_preferences.py uses the mozprofile executable which may not exist on all systems

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Jeff Hammel]

https://github.com/mozilla/mozbase/blob/master/mozprofile/tests/test_preferences.py
uses the 'mozprofile' script which is installed in setup.py.  This
does not work on windows even when installed; it should be
mozprofile.exe. (This also tells you how often tests should on
windows: basically not at all.) In mozilla-central we add the mozbase
packages via .pth files and not running `setup.py` at all:
https://wiki.mozilla.org/Auto-tools/Projects/Mozbase#Mozbase_in_Mozilla-Central
. This blocks bug 790765 .

Ideally, we should not rely on the executable at all.  Barring that,
we should at least check for mozprofile and mozprofile.exe and fail in
the class test setup function (this also requires `which` which we
don't have currently). In the m-c case, we would also have to disable
the test or install via setup.py.

Comment 1

[Jeff Hammel]

We may be able to get away with using the CLI entry point function: https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/cli.py#L80 . I'm fairly unsure if this is what we actually want to do or if there will be issues wrt using this vs spawning a subshell

Comment 2

[Jeff Hammel]

I'm going to go ahead and pursue the path of using mozprofile.cli.cli vs subshelling to mozprocess. This is somewhat scary as if we have code that does e.g. sys.exit or similar that is appropriate for CLI code but inappropriate for testing we're going to be dealing with odd or undefined behaviour.  Casual glances indicate no such code at this time: https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/cli.py

If anyone believes we shouldn't do this or has a better way of solving the problem, I'd be glad to hear them.  I do want to avoid anything that either copy+pastes code so that we're pretending a separation of concerns between mozprofile and its tests or has fragile checks against output that could easily break on code change.  While it is not the best practice to use (parts of) mozprofile to test (somewhat separate parts of) mozprofile, I can't think of a better way to do this that doesn't introduce either (possibly heavy) dependencies or introduce even worse problems.

Comment 3

[Jeff Hammel]

Attachment: use CLI front end

Comment 4

[Jeff Hammel]

if attachment 710401 [details] [diff] [review] lands, https://bugzilla.mozilla.org/show_bug.cgi?id=837887 can be closed

Comment 5

[Jonathan Griffin (:jgriffin)]

Comment on attachment 710401 [details] [diff] [review]
use CLI front end

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

lgtm, either with or without changes to reading cli's stdout.  It doesn't seem likely that the differences between subshelling and using mozprofile.cli directly will bite us, IMO.

::: mozprofile/tests/test_preferences.py
@@ +19,4 @@
>          """
> +
> +        # The CLI entry point prints the profile path to created
> +        # profiles to stdout.  In order to get this, monkey-patch sys.stdout

This seems slightly awkward; is there any reason not to return profile.profile from mozprofile.cli.cli?  Or, alternately, not to make a factory method to do most of what mozprofile.cli.cli does, that could return the profile object and/or path?

Comment 6

[Jeff Hammel]

(In reply to Jonathan Griffin (:jgriffin) from comment #5)
> Comment on attachment 710401 [details] [diff] [review]
> use CLI front end
> 
> Review of attachment 710401 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm, either with or without changes to reading cli's stdout.  It doesn't
> seem likely that the differences between subshelling and using
> mozprofile.cli directly will bite us, IMO.

No, either way ultimately this is using parts of mozprofile to test other parts of mozprofile.  Since there is overlap in code, this is never what I would call the best sort of test.  That said, I think these tests are important and that any 3rd party confirmation would be very heavy handed, though I'm up for suggestions.  Even if we had a 3rd party preference parser, we'd still probably want to keep these tests, IMHO.

> ::: mozprofile/tests/test_preferences.py
> @@ +19,4 @@
> >          """
> > +
> > +        # The CLI entry point prints the profile path to created
> > +        # profiles to stdout.  In order to get this, monkey-patch sys.stdout
> 
> This seems slightly awkward; is there any reason not to return
> profile.profile from mozprofile.cli.cli?  

So this shouldn't be done; since this is a setuptools console_script, returning anything other than the return code (or None, in this case), IIRC, will result in a failure when running the mozprofile script.  If we ever get a python page on how to make a package that is Mozilla- or at least A*Team-approved we should note this.  Its subtle and, IMHO, wrong, but that's how it works.

> Or, alternately, not to make a
> factory method to do most of what mozprofile.cli.cli does, that could return
> the profile object and/or path?

The real reason I didn't is because I couldn't figure out what that architecture looked like ;) Now, from the vantage point of of a few hours, I think I see the way.  Let me land this and post a follow-up.

Comment 7

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #3)
> Created attachment 710401 [details] [diff] [review]
> use CLI front end

pushed: https://github.com/mozilla/mozbase/commit/6c3d27f1b2107ae3b6d81496b9352f4741f387e9

follow up on its way

Comment 8

[Jeff Hammel]

Attachment: avoid crazy patching of sys.stdout that will inevitably bite us down the line

follow up; not a lot of code, but for whatever reason it took me a bit to figure out where to put things

Comment 9

[Jonathan Griffin (:jgriffin)]

Comment on attachment 710486 [details] [diff] [review]
avoid crazy patching of sys.stdout that will inevitably bite us down the line

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

Excellent, thanks!

Comment 10

[Jeff Hammel]

(In reply to Jonathan Griffin (:jgriffin) from comment #9)
> Comment on attachment 710486 [details] [diff] [review]
> avoid crazy patching of sys.stdout that will inevitably bite us down the line
> 
> Review of attachment 710486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Excellent, thanks!

And than(In reply to Jonathan Griffin (:jgriffin) from comment #9)
> Comment on attachment 710486 [details] [diff] [review]
> avoid crazy patching of sys.stdout that will inevitably bite us down the line
> 
> Review of attachment 710486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Excellent, thanks!

And tha(In reply to Jonathan Griffin (:jgriffin) from comment #9)
> Comment on attachment 710486 [details] [diff] [review]
> avoid crazy patching of sys.stdout that will inevitably bite us down the line
> 
> Review of attachment 710486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Excellent, thanks!

And thank you for the quick review :)

pushed: https://github.com/mozilla/mozbase/commit/c3dcd8c730b4a51fc8a0b80c2ba110510c1502b4

Comment 12

[Jeff Hammel]

Very strange:

https://github.com/mozilla/mozbase/blob/master/mozprofile/tests/test_preferences.py#L217

I missed one and yet tests all pass o_O . Looking now, but so far the correct code paths are being hit....

Comment 13

[Jeff Hammel]

Reopening, though hopefully not for long.  ::sigh:: for last minute discoveries...

Comment 14

[Jeff Hammel]

So this is not a bug as such ;)

What is happening is that mozprofile.cli stores self.args on the MozProfileCLI instance but then never uses them: https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/cli.py#L29 .  There are historical reasons for doing so, but no longer applicable. I'll file a bug about a short term fix, but really the solution is the long ago proposed general CLI class for mozbase.  I'll also put a one-line fix up for review, since while the test ain't broken, this confused me and would probably confuse most people.

Comment 15

[Jeff Hammel]

Attachment: follow up: change command line to be less confusing

Some days I just love our review policy o_O

Comment 16

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #14)
> So this is not a bug as such ;)
> 
> What is happening is that mozprofile.cli stores self.args on the
> MozProfileCLI instance but then never uses them:
> https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/cli.
> py#L29 .  There are historical reasons for doing so, but no longer
> applicable. I'll file a bug about a short term fix, but really the solution
> is the long ago proposed general CLI class for mozbase.  I'll also put a
> one-line fix up for review, since while the test ain't broken, this confused
> me and would probably confuse most people.

bug 839843 ; feel free to ignore my ranting

Comment 17

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #15)
> Created attachment 712224 [details] [diff] [review]
> follow up: change command line to be less confusing
> 
> Some days I just love our review policy o_O

pushed: https://github.com/mozilla/mozbase/commit/57d097a48e5855374998b7582beea9cdbd8fca8b

Thanks for the quick review, :jgriffin.