Closed Bug 851292 Opened 8 years ago Closed 8 years ago

Mozprofile pacURL in permissions.py not formatted properly for mochitests

Categories

(Testing :: Mozbase, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(1 file)

This is a regression I caused a while back in https://github.com/mozilla/mozbase/commit/ddab19ab677652d23023c6f11752c86163037da3

The motivation for the above patch was that I needed to specify the exact same information twice in two different method calls in the same module when proxying for peptest. However it appears that with mochitest, the address being proxied isn't the primary location (as defined in server-locations.txt) like it is with peptest.
This patch caters to both the peptest and mochitest use cases. It will use the remote specified by the primary location by default, but also gives consumers a chance to pass in their own remote values if needed (like in the mochitest case).

Works with both peptest and mochitest locally.
Attachment #725121 - Flags: review?(jhammel)
Comment on attachment 725121 [details] [diff] [review]
Patch 1.0 - allow user specified proxy information

I didn't do a thorough audit but I believe this is good.

+    def pac_prefs(self, user_proxy={}):

In general, I caution (and usually r-) passing mutable values as defaults.  It is actually okay IFF you never mutate them.  You don't here, so this is fine.  Still... If you wanted to fix (and suffer the extra line of code), feel free/encouraged.
Attachment #725121 - Flags: review?(jhammel) → review+
Pushed to master with the mutable fix:
https://github.com/mozilla/mozbase/commit/43f9510e3d58bfed32790c82a57edac5f928474d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Caused bustage, need to update the tests: https://travis-ci.org/mozilla/mozbase/builds/5725257
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://github.com/mozilla/mozbase/commit/0d240e02eb0c1149d4830091a7405cef029c8bec

The tests actually caught a corner case I had forgotten about. Hooray for tests!
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.