Closed Bug 1352269 Opened 7 years ago Closed 7 years ago

Implement Verify Max Concurrent Tab Count counts the number of opened tabs with telemetry harness

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: Silne30, Assigned: Silne30)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file)

Blocks: 1352196
Priority: -- → P3
Whiteboard: [measurement:client]
Attachment #8853493 - Flags: review?(alessio.placitelli)
Assignee: nobody → jdorlus
Status: NEW → ASSIGNED
Comment on attachment 8853493 [details]
Bug 1352269 - Added tab scalar test and manifest file

I think Georg has better context to review this, bouncing the review to him!
Attachment #8853493 - Flags: review?(alessio.placitelli) → review?(gfritzsche)
Comment on attachment 8853493 [details]
Bug 1352269 - Added tab scalar test and manifest file

https://reviewboard.mozilla.org/r/125574/#review129350

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:57
(Diff revision 1)
> -            self.logger.info(self.ping_server_url)
> +            if method == 'install':
> +                # The install must be performed asynchronously
> -            trigger = Process(target=self.trigger_ping, args=(method,))
> +                trigger = Process(target=self.trigger_ping, args=(method,))
> -            trigger.start()
> +                trigger.start()
> +            else:
> +                self.trigger_ping(method)

I think i mentioned a few times that we want to trigger those from the tests.

Each test should decide on its own what actions it will take and whether it expects a ping or not.
I still don't think it makes sense to centralize this.

From tests i want to:
- perform some actions
- wait for N resulting pings

It makes sense to e.g. have a shared helper function to install an addon, but not to hard-couple the whole logic in here.

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:93
(Diff revision 1)
> +        return self.marionette.execute_script('Cu.import("resource://gre/modules/TelemetrySession.jsm");'
> +                                              'return TelemetrySession.getMetadata();')[u'subsessionId']

`TelemetryController.getCurrentPingData(true)` is the most "public" exposed function we have for getting this.

Please make this more readable too.
```data = ...
return data[...][...]```

::: toolkit/components/telemetry/tests/marionette/tests/client/manifest.ini:4
(Diff revision 1)
> +[DEFAULT]
> +tags = client
> +
> +[test_main_ping_addon_install_tab_window_scalars.py]

Does the name need to be that long?

::: toolkit/components/telemetry/tests/marionette/tests/client/test_main_ping_addon_install_tab_window_scalars.py:19
(Diff revision 1)
> +        assert ping['payload']['processes']['parent']['scalars']['browser.engagement.max_concurrent_tab_count'] == 3
> +        assert ping['payload']['processes']['parent']['scalars']['browser.engagement.tab_open_event_count'] == 2
> +        assert ping['payload']['processes']['parent']['scalars']['browser.engagement.max_concurrent_window_count'] == 1

Why not use a local `scalars` variable to make this more readable?

::: toolkit/components/telemetry/tests/marionette/tests/unit/test_ping_server_received_ping.py:17
(Diff revision 1)
> -        r = requests.post(self.ping_server_url, data=json.dumps(data), headers=headers)
> -        assert r.status_code == 200
> +        req = requests.post(self.ping_server_url, data=json.dumps(data), headers=headers)
> +        assert req.status_code == 200

This seems unrelated?
Attachment #8853493 - Flags: review?(gfritzsche)
Comment on attachment 8853493 [details]
Bug 1352269 - Added tab scalar test and manifest file

https://reviewboard.mozilla.org/r/125574/#review129350

> Does the name need to be that long?

Yes. Because we have so many test cases and they are so similar, the names have to be granular. If the eventual goal is a test suite, the names will have to have meaningful differences. From the name, we can tell how the ping is generated and what is tested.
Comment on attachment 8853493 [details]
Bug 1352269 - Added tab scalar test and manifest file

https://reviewboard.mozilla.org/r/125574/#review129350

> This seems unrelated?

Yes, and no. This variable change came because I was debugging and r is a global variable. I didn't think I should have a separate patch for a variable change. What are your thoughts?
Comment on attachment 8853493 [details]
Bug 1352269 - Added tab scalar test and manifest file

https://reviewboard.mozilla.org/r/125574/#review129350

> Yes, and no. This variable change came because I was debugging and r is a global variable. I didn't think I should have a separate patch for a variable change. What are your thoughts?

It resulted from a merge with mozilla-central.
Comment on attachment 8853493 [details]
Bug 1352269 - Added tab scalar test and manifest file

https://reviewboard.mozilla.org/r/125574/#review129350

> I think i mentioned a few times that we want to trigger those from the tests.
> 
> Each test should decide on its own what actions it will take and whether it expects a ping or not.
> I still don't think it makes sense to centralize this.
> 
> From tests i want to:
> - perform some actions
> - wait for N resulting pings
> 
> It makes sense to e.g. have a shared helper function to install an addon, but not to hard-couple the whole logic in here.

Each test does decide on it's own whether it expects a ping or not. The trigger is in the tests but having each test write it's own implementation of waiting will be a duplication of code and make the tests harder to read/debug/maintain. Remember, we are using the marionette harness to build and run these tests so our test code will be structured like marionette tests are. If you are looking to write a new harness from scratch, that may be another issue but isn't that what Andre had done and we were advised to use what is established? Can you show me an example of what the test would actually look like that you are proposing? (in pseudocode)
I put this in other notes before. Inside the test i'd write:

> do_something_that_triggers_ping()
> # ... maybe do other things.
> ping = wait_for_pings()
Comment on attachment 8853493 [details]
Bug 1352269 - Added tab scalar test and manifest file

https://reviewboard.mozilla.org/r/125572/#review130040

::: toolkit/components/telemetry/tests/marionette/tests/client/test_main_ping_addon_install_tab_window_scalars.py:17
(Diff revision 3)
> +            self.browser.tabbar.open_tab()
> +            self.browser.tabbar.open_tab()
> +            self.browser.tabbar.close_tab()
> +            self.browser.tabbar.close_tab()
> +
> +        self.trigger_ping('install')

Separated the triggering of a ping from the waiting for the ping.
(In reply to John Dorlus [:Silne30] from comment #5)
> Yes, and no. This variable change came because I was debugging and r is a
> global variable. I didn't think I should have a separate patch for a
> variable change. What are your thoughts?

It's an unrelated change in a file that is not touched otherwise.
A separate commit makes reviewing and approving that trivial.
(In reply to John Dorlus [:Silne30] from comment #4)
> Comment on attachment 8853493 [details]
> Bug 1352269 - Added tab scalar test and manifest file
> 
> https://reviewboard.mozilla.org/r/125574/#review129350
> 
> > Does the name need to be that long?
> 
> Yes. Because we have so many test cases and they are so similar, the names
> have to be granular. If the eventual goal is a test suite, the names will
> have to have meaningful differences. From the name, we can tell how the ping
> is generated and what is tested.

I don't agree with that reasoning.
We have many tests in the tree and i have not seen test names that long before.
You should find something shorter and more succinct.
Comment on attachment 8853493 [details]
Bug 1352269 - Added tab scalar test and manifest file

https://reviewboard.mozilla.org/r/125572/#review130456

What is easyscreenshot.xpi?
Where does it come from?
Where is that documented?
Are there any other addons in-tree already that you can use?
If not, why not use a trivial "no-op" / "test" addon?

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:57
(Diff revision 3)
> -        current_num_pings = len(TelemetryTestCase.ping_list)
> +        current_num_pings = len(self.ping_list)
>          try:
> -            self.logger.info(self.ping_server_url)
> -            trigger = Process(target=self.trigger_ping, args=(method,))
> -            trigger.start()
>              Wait(self.marionette, 60).until(lambda t: len(self.ping_list) > current_num_pings)

This is racey:
- trigger ping
- do some stuff
- ping is received
- do some more stuff
- wait on ping

You need to handle the case of "we already have a ping".

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:62
(Diff revision 3)
> -            self.logger.debug('ping received')
>              return self.ping_list[-1]
>          except Exception as e:
>              self.fail('Error generating ping: {}'.format(e.message))
>  
>      def trigger_ping(self, method):

Why the "method" parameter?
These are just different utilities to trigger test or Firefox behavior.
Just break this into separate functions.

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:88
(Diff revision 3)
>          return self.marionette.execute_script('Cu.import("resource://gre/modules/ClientID.jsm");'
>                                                'return ClientID.getCachedClientID();')
>  
> +    @property
> +    def subsession_id(self):
> +        meta_data = self.marionette.execute_script(

ping_data

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:91
(Diff revision 3)
> +    @property
> +    def subsession_id(self):
> +        meta_data = self.marionette.execute_script(
> +            'Cu.import("resource://gre/modules/TelemetryController.jsm");'
> +            'return TelemetryController.getCurrentPingData(true);')
> +        return meta_data['payload']['info'][u'subsessionId']

Why is one u'' and the other not?

::: toolkit/components/telemetry/tests/marionette/tests/client/test_main_ping_addon_install_tab_window_scalars.py:19
(Diff revision 3)
> +            self.browser.tabbar.close_tab()
> +            self.browser.tabbar.close_tab()
> +
> +        self.trigger_ping('install')
> +        ping = self.wait_for_ping()
> +        scalars = ping['payload']['processes']['parent']['scalars']

You'll want to move this to after asserting this is a main ping.
Probably to just before accessing the scalars.

::: toolkit/components/telemetry/tests/marionette/tests/unit/test_ping_server_received_ping.py:17
(Diff revision 3)
> -        r = requests.post(self.ping_server_url, data=json.dumps(data), headers=headers)
> -        assert r.status_code == 200
> +        req = requests.post(self.ping_server_url, data=json.dumps(data), headers=headers)
> +        assert req.status_code == 200

Unrelated.
This belongs into a separate commit.
Attachment #8853493 - Flags: review?(gfritzsche)
Comment on attachment 8853493 [details]
Bug 1352269 - Added tab scalar test and manifest file

https://reviewboard.mozilla.org/r/125574/#review131860

This looks pretty close.

The patch doesn't apply cleanly on recent mozilla-central though.
Is there any other code that's not landed yet?

Also, we started linting our Python files recently, you should run this and fix any issues in your files:
`./mach lint -l flake8 toolkit/components/telemetry`

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:62
(Diff revisions 3 - 4)
> -            return self.ping_list[-1]
> -        except Exception as e:
> +            except Exception as e:
> -            self.fail('Error generating ping: {}'.format(e.message))
> +                self.fail('Error generating ping: {}'.format(e.message))
> +        return self.ping_list.pop()
>  
> -    def trigger_ping(self, method):
> +    def change_update_pref(self):

`toggle_update_pref(self)`

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:63
(Diff revisions 3 - 4)
>              self.marionette.enforce_gecko_prefs({'app.update.enabled': False}) \
>                  if self.marionette.get_pref('app.update.enabled') \
>                  else self.marionette.enforce_gecko_prefs({'app.update.enabled': True})

Make this more readable:
```
value = self.marionette.get_pref(...)
self.marionette.enforce_gecko_prefs({'...': not value})
```
Attachment #8853493 - Flags: review?(gfritzsche)
Comment on attachment 8853493 [details]
Bug 1352269 - Added tab scalar test and manifest file

https://reviewboard.mozilla.org/r/125574/#review131860

> Make this more readable:
> ```
> value = self.marionette.get_pref(...)
> self.marionette.enforce_gecko_prefs({'...': not value})
> ```

Nice! Fixed!
Attachment #8853493 - Flags: review?(chutten)
Comment on attachment 8853493 [details]
Bug 1352269 - Added tab scalar test and manifest file

https://reviewboard.mozilla.org/r/125574/#review132562

I don't have much experience with marionette, but I had a few things to say of questionable usefullness...

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:71
(Diff revision 6)
> +    def restart_browser(self):
> +        """Restarts browser while maintaining the same profile and session."""
> +        self.marionette.restart(clean=False, in_app=True)
>  
> -    def trigger_ping(self, method):
> -        if method == 'pref':
> +    def install_addon(self):
> +            trigger = Process(target=self._install_addon)

Isn't this tabbed too far in? (are you running any linting?)

::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py:81
(Diff revision 6)
> +        # It has high compatibility with firefox and doesn't cause any adverse side affects that
> +        # could affect our tests like tabs opening, etc.
> +        # Developed by: MozillaOnline
> +        # Addon URL: https://addons.mozilla.org/en-US/firefox/addon/easyscreenshot/
>          try:
> -            addon_path = os.path.join(resources_dir, 'abp_2.8.2.xpi')
> +            addon_path = os.path.join(resources_dir, 'easyscreenshot.xpi')

Does that mean you can remove abp_2.8.2.xpi?

::: toolkit/components/telemetry/tests/marionette/tests/client/test_main_tab_scalars.py:15
(Diff revision 6)
> +    def test_main_tab_scalars(self):
> +        with self.marionette.using_context(self.marionette.CONTEXT_CHROME):
> +            tab2 = self.browser.tabbar.open_tab()
> +            self.browser.tabbar.switch_to(tab2)
> +            tab3 = self.browser.tabbar.open_tab()
> +            self.browser.tabbar.switch_to(tab3)

Do you have to switch tabs?
Attachment #8853493 - Flags: review?(chutten) → review-
Comment on attachment 8853493 [details]
Bug 1352269 - Added tab scalar test and manifest file

https://reviewboard.mozilla.org/r/125574/#review132562

> Isn't this tabbed too far in? (are you running any linting?)

I ran the mach linting command that Georg gave me. There were no issues.

> Does that mean you can remove abp_2.8.2.xpi?

Yes. That file should not be in tree anymore.

> Do you have to switch tabs?

Yes. Actually, I did not have the tab switches before. But Henrik (:whimboo), who maintains marionette, let me know that the tab switches are necessary.
Comment on attachment 8853493 [details]
Bug 1352269 - Added tab scalar test and manifest file

https://reviewboard.mozilla.org/r/125574/#review132562

> I ran the mach linting command that Georg gave me. There were no issues.

The linter complains if indentation isn't a multiple of 4 space. In this case is 8 spaces, which is a multiple of 4.. but it's too many spaces :-) You just need 4 spaces here.
Comment on attachment 8853493 [details]
Bug 1352269 - Added tab scalar test and manifest file

https://reviewboard.mozilla.org/r/125574/#review132562

> The linter complains if indentation isn't a multiple of 4 space. In this case is 8 spaces, which is a multiple of 4.. but it's too many spaces :-) You just need 4 spaces here.

Fixed and pushed.
Attachment #8853493 - Flags: review?(gfritzsche) → review?(alessio.placitelli)
Comment on attachment 8853493 [details]
Bug 1352269 - Added tab scalar test and manifest file

https://reviewboard.mozilla.org/r/125574/#review132616
Attachment #8853493 - Flags: review?(chutten) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 1e12d557a8d0 -d 7d1429fe13f7: rebasing 389691:1e12d557a8d0 "Bug 1352269 - Added tab scalar test and manifest file r=chutten" (tip)
merging toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py
warning: conflicts while merging toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Attachment #8853493 - Flags: review?(alessio.placitelli)
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea85026945a7
Added tab scalar test and manifest file r=chutten
Attachment #8853493 - Flags: review?(gfritzsche)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Did not mean to mark as fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/ea85026945a7
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: