Mozmill should not be allowed to modify the profile_args and runner_args objects from caller

RESOLVED FIXED

Status

Testing Graveyard
Mozmill
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

unspecified
Bug Flags:
in-testsuite +

Details

(Whiteboard: [mozmill-2.0.1+])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
As what I have seen while working on mozprofile and some testing with out automation scripts, Mozmill messes around with the callers profile_args and runner_args. This is somewhat bad and can cause unpredicted application behavior, beside duplicated entries in lists. In the following an example:

There is some code which makes use of the Mozmill create() method, and which handles a list of add-ons on its own. As of now Mozmill modifies this original list of add-ons:

    def some_method():
        self.addon_list = []

        profile_args = dict(addons=self.addon_list)
        runner_args = dict(binary=self._application)
        mozmill_args = dict(profile_args=profile_args,
                            runner_args=runner_args)
        
        self._mozmill = mozmill.MozMill.create(**mozmill_args)
        self._mozmill.run(tests)

        print self.addon_list

For the final print I would expect to see an empty list, but instead you currently get:

['/mozilla/code/mozmill/mozmill/mozmill/extension', '/mozilla/code/mozmill/jsbridge/jsbridge/extension']

That means Mozmill itself modifies variables from other tools. I don't think that we should do that, and better do a deepcopy of those passed in references to operate on our own object. Really, if an API has to return a modified version of that object, it should be done via a getter or return value but not silently.
(Assignee)

Comment 1

4 years ago
Created attachment 828619 [details] [diff] [review]
Patch v1

Andrew, please check my initial comment on this bug for the description. This patch is not finalized yet, given that it misses a real test. For now I would like to know if that is the correct route to go. I don't see a better one, without moving too much overhead to the consumers.
Attachment #828619 - Flags: feedback?(ahalberstadt)
(Assignee)

Updated

4 years ago
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 828619 [details] [diff] [review]
Patch v1

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

This makes sense to me, and looks like the right approach. Was this actually causing a problem?
Attachment #828619 - Flags: feedback?(ahalberstadt) → feedback+
(Assignee)

Comment 3

4 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #2) 
> This makes sense to me, and looks like the right approach. Was this actually
> causing a problem?

Yes, i have noticed that while working on the add-on stuff in mozbase and by using our automation scripts, which in some cases create a second instance of Mozmill to run another set of tests. Here our list of add-ons has been manipulated by Mozmill so the mozmill and jsbridge extension were listed even twice at the end. With that I also found bug 935436 which is fixed now.

Thanks for your feedback! I will create a proper mutt test for it.
(Assignee)

Comment 4

4 years ago
Created attachment 828845 [details] [diff] [review]
Patch v1.1

Patch with tests for both profile and runner args.
Attachment #828619 - Attachment is obsolete: true
Attachment #828845 - Flags: review?(ahalberstadt)
Comment on attachment 828845 [details] [diff] [review]
Patch v1.1

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

This looks good, though it doesn't have the change from the previous patch. I've already f+'ed it though, so this r+ applies to everything.
Attachment #828845 - Flags: review?(ahalberstadt) → review+
(Assignee)

Comment 6

4 years ago
Created attachment 829338 [details] [diff] [review]
patch v2

That's happening when you don't do a rebase before the format-patch command. Sorry for that. Here the complete patch with the formerly feedbacked code change by taking over r+.
Attachment #828845 - Attachment is obsolete: true
Attachment #829338 - Flags: review+
(Assignee)

Comment 7

4 years ago
Landed as:
https://github.com/mozilla/mozmill/commit/239786f45c59024efa8513f48eef53bdc6cabfe5 (master)
https://github.com/mozilla/mozmill/commit/7648d6623196b7ea12a10fe8b40085dc90bb51a3 (hotfix-2.0)
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [mozmill-2.0.1?] → [mozmill-2.0.1+]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.