777168 - update tests.py with docstrings from https://wiki.mozilla.org/Buildbot/Talos

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jeff Hammel]

Due to jmaher's heroic documentation efforts, we now have descriptions
to most of the our tests at https://wiki.mozilla.org/Buildbot/Talos .
I'd like to move them into tests.py and have that be the canonical
source.  A few reasons:

- these could be printed with --print-tests so you can actually see
  what you're testing

- being in-code, they can *actually* be up to date and current

- it is relatively easy to build something like
  https://wiki.mozilla.org/Buildbot/Talos from code, and fairly
  impossible to go the other direction

Comment 1

[Johannes Vogel (:nebelhom)]

Just stumbled over this bug here (There weren't any beginner's bugs that took my fancy). Not sure if it is still up to date and/or if I have the ability to fix it. It doesn't "look" too bad... What do you think?

As usual, I would probably need some pointers before I start this.

Comment 2

[Burak Yiğit Kaya [:BYK]]

(In reply to Johannes Vogel (:nebelhom) from comment #1)
> Just stumbled over this bug here (There weren't any beginner's bugs that
> took my fancy). Not sure if it is still up to date and/or if I have the
> ability to fix it. It doesn't "look" too bad... What do you think?
> 
> As usual, I would probably need some pointers before I start this.

Ping me when you have time to give it a shot!

Comment 3

[Jeff Hammel]

Currently, talos/PerfConfigurator have a --print-tests option on the command line:

(talos)│talos --print-tests
Available tests:
a11yr
dromaeo_css
dromaeo_dom
kraken
sunspider
tcheck2
tcheck3
tcheckerboard
tdhtml
tdhtmlr
tp4m
tp5n
tpaint
tprovider
tresize
trobopan
ts
ts_paint
tscrollr
tspaint_places_generated_max
tspaint_places_generated_med
tsvg
tsvgr
tsvgr_opacity
v8_7

It would be nice to have this print a short description of the information, e.g.

tp5n: Tests the time it takes Firefox to load the tp5 web page test set.

(summary from https://wiki.mozilla.org/Buildbot/Talos#tp5 )

Likewise, if you have --activeTests specified, talos prints the parameters for the running tests:

(talos)│talos --activeTests ts --print-tests
- cycles: 20
  name: ts
  shutdown: true
  timeout: 150
  url: startup_test/startup_test.html?begin=
  url_timestamp: true

Again, it would be nice to have a possibly longer description for this information.

Probably the first thing to do is to get the information from https://wiki.mozilla.org/Buildbot/Talos to docstrings in http://hg.mozilla.org/build/talos/file/tip/talos/test.py . It certainly doesn't have to be all the information for round 1: this is very much a proof of concept and we're not going to be able to transition from https://wiki.mozilla.org/Buildbot/Talos being canon anytime soon.  That said, in general there is a movement to Mozilla to move documentation closer to what is being documented and having e.g. wiki, etc, being built from the in-source info.

For the first round, decent docstrings in hg.mozilla.org/build/talos/file/tip/talos/test.py from the information in https://wiki.mozilla.org/Buildbot/Talos and modifying --print-tests to display summaries from this information would be a big help.

Comment 4

[Johannes Vogel (:nebelhom)]

Ok, I gave it a first try and it is definitely doable. Before I go any further, a couple of things to note:

1. The information on https://wiki.mozilla.org/Buildbot/Talos is far from complete. Due to my ignorance, I was only able to fill in about 70% of the Tests. For example, I found descriptions for tsvg test but not for tsvgr.
Same for ts_paint but not really for ts.

2. Also some of these descriptions aren't particularly technical:
e.g.
https://wiki.mozilla.org/Buildbot/Talos#tscroll Quote description: This test does some scrolly thing.
This amused me greatly, but it doesn't really help ;)

Given the gaps and incomplete documentation, is it worth trying to do this now or better wait until the documentation is finalised?

3. Lastly, I noticed that there is a general division into tests inheriting from TsBase and from Pageloader tests. With a bit of playing around I may be able to create two tables. Each under a distinct heading with description of the general procedure by trying to divide by looking for subclasses. My question: is it worth doing?

Comment 5

[Jeff Hammel]

Thanks for looking into this.  Replies inline:

(In reply to Johannes Vogel (:nebelhom) from comment #4)
> Ok, I gave it a first try and it is definitely doable. Before I go any
> further, a couple of things to note:
> 
> 1. The information on https://wiki.mozilla.org/Buildbot/Talos is far from
> complete. Due to my ignorance, I was only able to fill in about 70% of the
> Tests. For example, I found descriptions for tsvg test but not for tsvgr.
> Same for ts_paint but not really for ts.

Yeah, some info is definitely missing from the page.  Others, like tsvgr, are there but you'll need to use text-search and infer what it it is.  For instance, tsvgr is "row-based tsvg" which you can see from https://wiki.mozilla.org/Buildbot/Talos#tsvg . For this sort of thing, I am in general fine documenting the "generic" case of e.g. tsvg and moving on for a few reasons:

- it will be better to have something that can be refined later than nothing

- ultimately, these tests are the same, they just have different run-time parameters, which if you run e.g. talos --print-tests -a nameOfTest you can see:

- name: tsvg
  tpcycles: 5
  tpmanifest: ${talos}/page_load_test/svg/svg.manifest
(talos)│talos --print-tests -a tsvgr
- name: tsvgr
  tpcycles: 1
  tpmanifest: ${talos}/page_load_test/svg/svg.manifest
  tppagecycles: 25
(talos)│

We also ultimately want to change how tests are defined/run into something much different, someday. https://bugzilla.mozilla.org/show_bug.cgi?id=814228 hints at some longer-term direction, but certainly we want to remove test runtime parameters from test definitions.

That said, if you want to add variant-specific docstrings for the time being, that would be appreciated too. Obviously nothing we do to the docstrings here is Set in Stone.

> 2. Also some of these descriptions aren't particularly technical:
> e.g.
> https://wiki.mozilla.org/Buildbot/Talos#tscroll Quote description: This test
> does some scrolly thing.
> This amused me greatly, but it doesn't really help ;)

Well, it *does* do a scrolly thing ;)

> Given the gaps and incomplete documentation, is it worth trying to do this
> now or better wait until the documentation is finalised?

My general opinion is "yes", though if :jmaher has any thoughts to the contrary, I'd like to hear.  The reasons for my opinion:

- believe it or not, https://wiki.mozilla.org/Buildbot/Talos has had much work done on it this year. It is likely as close to "complete" as it will ever be.  Along those lines...

- documentation is an ongoing s/burden/effort/ ; it will never be done and it will never be perfect.  Even worse than code, I know!

- having documentation available in multiple places will alert more classes of users as to the documentation's short-comings.  This starts discussions, like this one, which hopefully leads to the documentation being fixed :) (And yes, incidentally, we should fix https://wiki.mozilla.org/Buildbot/Talos#tscroll .)  A really nice thing to do would be to note a list of things that don't make sense/are misleading/are missing from https://wiki.mozilla.org/Buildbot/Talos . If you wanted to fix them and have the knowledge to do so (for some of them anyway), that would be bonus points, but even keeping track of them and ticketing them is probably 80% of the effort of getting them fixed.

> 3. Lastly, I noticed that there is a general division into tests inheriting
> from TsBase and from Pageloader tests. With a bit of playing around I may be
> able to create two tables. Each under a distinct heading with description of
> the general procedure by trying to divide by looking for subclasses. My
> question: is it worth doing?

IMHO, no, for the reason that I (personally, at least) would like to make the classes of tests less disparate vs more.  There are really at least three categories: "startup" tests (which really means having a URL), pageloader tests which use the pageloader extension to accrue numbers, pageloader tests which accrue their own numbers...probably robocop tests are a fourth category...there have (and probably will be again) addon tests...etc. I can only imagine us having more categories going forward. So I would hold off on putting any effort into separating their display in --print-tests until we can figure out https://bugzilla.mozilla.org/show_bug.cgi?id=814228 and similar.  That said, if :jmaher believes this would be of practical use, I completely reverse my opinion ;)

Comment 6

[Jeff Hammel]

see also: https://bugzilla.mozilla.org/show_bug.cgi?id=797186 wrt comment 5

Comment 7

[Johannes Vogel (:nebelhom)]

Attachment: First attempt

Ok, here is the first attempt.

Things to note:
1. The main addition is the 'description' method in the Test base class. The format only buggers up slightly if there is no docstring (it just prints a None), which is the reason for the last 'print "\n"' statement

2. Someone who understands all these tests needs to read through the docstrings. I cannot guarantee the correctness of all of them.

3. I couldn't find docstrings for the following tests:

- trobopan
- tcheckerboard
- tprovider
- tcheck2
- tcheck3
(http://hg.mozilla.org/build/talos/file/tip/talos/test.py#l82 and onwards)

As a consequence, I only added

"""
A mobile ts_type test (docstring to be updated)
"""

4. If you don't like the underlining of the tests and all, let me know. I was doodling around.

5. The formatting for the call to 'talos --print-tests -a NameOfTest' is not exactly ideal, but I can't do any better. I would have liked description to come after the name, but anything short of changing the yaml probably wouldn't do it, but I guess, we talked about that in IRC.

Let me know what you think. thanks

Comment 8

[Jeff Hammel]

This looks basically good but it does not currently apply for me:

(talos)│curl https://bug777168.bugzilla.mozilla.org/attachment.cgi?id=687325 | patch -p1 --dry-run
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 16312  100 16312    0     0  77394      0 --:--:-- --:--:-- --:--:--   98k
patching file talos/PerfConfigurator.py
patching file talos/test.py
Hunk #2 FAILED at 41.
1 out of 2 hunks FAILED -- saving rejects to file talos/test.py.rej
(talos)│

Could you make it so it can apply, please?  I'd like to examine the output before reviewing

Comment 9

[Geoff Brown [:gbrown]]

(In reply to Johannes Vogel (:nebelhom) from comment #7)
> Created attachment 687325 [details] [diff] [review]
> 3. I couldn't find docstrings for the following tests:
> 
> - trobopan
> - tcheckerboard
> - tprovider
> - tcheck2
> - tcheck3
> (http://hg.mozilla.org/build/talos/file/tip/talos/test.py#l82 and onwards)
> 
> As a consequence, I only added

I have very simple descriptions for some of those tests in http://gbrownmozilla.wordpress.com/2012/11/30/mobile-firefox-performance-measures-november-check-up/ -- might be helpful.

Comment 10

[Johannes Vogel (:nebelhom)]

Attachment: Second

I forget to call hg pull -u before I created the patch. Sorry for the schoolboy error. Here is the next attempt with the additional information from gbrown (Thanks by the way!)

I hope everything is fine now. It works on my machine as expected... I won't be available until next week from now on as something important has come up. I hope it can wait until then. I don't like leaving you high and dry.

Sorry about that.

Comment 11

[Jeff Hammel]

Comment on attachment 687866 [details] [diff] [review]
Second

+                test_class_names = [(test_class.name(), test_class.description()) 

Should .strip() this description too to avoid the extra newlines.  This is also highly dependent on the formatting of the docstring in the files.  It might be better to do:

description_lines = [i.strip() for i in description.strip().splitlines()]

+    @classmethod
+    def description(cls):
+        return cls.__doc__

(Could conceivably put more of the formatting here)

Other than that, works great!  I'd still like to see one more pass for the formatting issue, unless :jmaher has differing thoughts, so I'll give an r- for now.

Comment 12

[Johannes Vogel (:nebelhom)]

Attachment: Third

Ok, here is the new version of it. changes to the previous version.

I extended the description classmethod, so that it is handled "prettier". this gave me the chance to incorporate jhammel's suggestion of using

description_lines = [i.strip() for i in description.strip().splitlines()]

while still avoiding an error from a call to None.strip() when the docstring is empty.

The last change is the removal of "\n" from the last print statement for each test. Took me ages to figure out that it gives two lines because the print statement already does that once... Well, to be honest, jhammel figured it out and told me :P

I hope it is up to standard. Thanks

Comment 13

[Jeff Hammel]

Comment on attachment 690927 [details] [diff] [review]
Third

--print-tests without any tests specified looks really good!

Unfortunately, --print-tests with --activeTests (or -a) specified only adds the description for the first test specified.  With the patch applied:

"""
(talos)│talos --print-tests -a ts:tsvg
- cycles: 20
  description: A basic start up test (ts = test start up) The overall test number
    is calculated by excluding the max opening time and taking an average of the remaining
    numbers. Unlike ts_paint, ts test uses a blank profile.
  name: ts
  shutdown: true
  timeout: 150
  url: startup_test/startup_test.html?begin=
  url_timestamp: true
- name: tsvg
  tpcycles: 5
  tpmanifest: ${talos}/page_load_test/svg/svg.manifest
(talos)│
"""

Note that ts has the description but tsvg does not (likewise, if I specify -a tsvg:ts the reverse is true).

Instead of:

+                description = ' '.join(test.test_dict[config_test_copy[0]['name']].description().strip().split())
+                config_test_copy[0]['description'] = description

You'll need to loop over all config_test_copy and set the descriptions for each one:
    for test_config in config_test_copy:
        description = ' '.join(test.test_dict[test_config['name']].description().strip().split())
        test_config['description'] = description

Comment 14

[Johannes Vogel (:nebelhom)]

Attachment: Fourth

Hmmm... I didn't consider the possibility that there is a possibility between one and all tests. Entirely slipped my mind.

Ah well, here's the next patch. Looped over each test given and added some visual separators for clarity's sake.

Let's hope that is is, then. Thanks

Comment 15

[Jeff Hammel]

Comment on attachment 691277 [details] [diff] [review]
Fourth

Looks great!  thanks for your patience on this one.

I'd like to do a lot more of this and in general we should have a strategy for test documentation going forward, but I'm kinda content to sit on this for now.  Hopefully this will be a great help to developers looking for help on the tests!

Comment 16

[Jeff Hammel]

Updated https://wiki.mozilla.org/Buildbot/Talos and various subpages.

Landed: http://hg.mozilla.org/build/talos/rev/7217b5b18353

I'm not sure what our strategy is going forward here -- we should come up with something before the in-code test descriptions and the wiki pages show large disparities.  Any ideas? Should revisit this issue, but I'll close for now.

Comment 17

[Johannes Vogel (:nebelhom)]

Hmmm... would it be cheating to look at the way other projects have done it?

Spontaneously, I would think of python's own documentation, for example. I am just throwing this out there without much knowledge about reStructuredText and Sphinx or the amount of work it would take to adapt it to the new system, just in case noone thought of this before ;)

Comment 18

[Jeff Hammel]

(In reply to Johannes Vogel (:nebelhom) from comment #17)
> Hmmm... would it be cheating to look at the way other projects have done it?

At Mozilla, ABICT it seems every project does this differently.  AFAIK, mozilla-central docs are not in any way autoposted to MDN (though this has often been discussed).  Really, I think we're mostly on our own here:  as much as I'd like to start discussing and then building a system that made sense across the board, that's a huge effort that I don't want to get into right now.

> Spontaneously, I would think of python's own documentation, for example. I
> am just throwing this out there without much knowledge about
> reStructuredText and Sphinx or the amount of work it would take to adapt it
> to the new system, just in case noone thought of this before ;)

Currently Talos test docs are in https://wiki.mozilla.org/Buildbot/Talos/Tests ; we *could* change where Talos documentation lives, but I'm also less inclined to do that, especially without some sort of convergence of documentation strategies at Mozilla.

I was envisioning something like the following:

- developer updates Talos test docs or adds a test
- there is a script in the talos repo that generates wiki.m.o appropriate docs and posts to the appropriate place on https://wiki.mozilla.org/Buildbot/Talos/Tests ; a developer runs this script and voila!
- on https://wiki.mozilla.org/Buildbot/Talos/Tests we boldly state somewhere that the descriptions (etc) are auto-generated and that that page shouldn't be edited and other information about the systems
- For Big Bonus Points, but much harder, a system whereby:
  * changes to the wiki page would be listened to (could we get this into pulse plz? kthx)
  * a bug would be written with the wiki changes that would apply to the Talos repo

IMHO, we should probably work across the board towards a system like this.  That said, I hesitate to do anything big (read: not the Big Bonus Points part) until Mozilla (development) as a whole starts consolidating on some solution. (I'm also ignoring a number of related documentation issues beyond the scope of this bug: why is wiki.m.o and MDN separate? How to move information easily between them? etc)

Comment 19

[Johannes Vogel (:nebelhom)]

Hmmm... I wasn't looking at the bigger picture. All I thought about was finding a way to automagically have a subset of the documentation which is created each time from the talos docstrings (so that changes are incorporated).

That way we at least have a place where all the documentation is always the same between online docs and documentation in the code.

Y'know, like you said. Start from a small increment and see if it catches on...