Closed Bug 1190376 Opened 7 years ago Closed 7 years ago

move the sps profilling stuff in another module


(Testing :: Talos, defect)

Not set


(Not tracked)



(Reporter: parkouss, Assigned: parkouss)




(1 file, 1 obsolete file)

Right now the sps profiling feature is coded into the TTest._runTest method.

We should move that code in his own file, to clearly separate concerns.

See the discussion in bug 1190270 around this.
Attached patch sps_profile.patch (obsolete) — Splinter Review
This move the sps_profile code in his own module.

Works good locally, with and without --spsProfile option.

pushed a try without sps here:

Maybe we should push one try with sps also ?
Assignee: nobody → j.parkouss
Attachment #8643314 - Flags: review?(jmaher)
Comment on attachment 8643314 [details] [diff] [review]

Review of attachment 8643314 [details] [diff] [review]:

I like this patch in general, but I don't like the:
from talos import utils
from talos.utils import TalosError
from talos.sps_profile import SpsProfile

I didn't think we had talos as a module?
Attachment #8643314 - Flags: review?(jmaher) → review-
We have now, with bug 1189021. :)
I forgot!  How will this work locally?
Just fine, since talos is installed (so the package talos exists locally), and "import talos" already worked there.

This module thing was only an issue with harness, since we are not installing talos - but the dependencies on harness. But this has been fixed, so we can use real nice imports now.
does this imply that you run talos from a specific directory?  I think it is ok, but I want to confirm before changing to a r+.

Also have you done a push to try with --spsProfiling enabled?
(In reply to Joel Maher (:jmaher) from comment #6)
> does this imply that you run talos from a specific directory?  I think it is
> ok, but I want to confirm before changing to a r+.

Well, no, this is not required. Not for me at least, as I use the talos 'binary' that is provided. Also no just if the user uses a virtualenv (like what is done using So I think this is safe from that point of view.

> Also have you done a push to try with --spsProfiling enabled?

Not yet! I was wondering what would be the appropriate syntax. Maybe exactly the one you provided in bug 1190270 ? Also the try here is pretty unreadable, because of the reasons explained by mail. The fix in bug 1190907 should do the trick for now, so I am waiting for it to be landed before repushing to try here - I think a bit with and whithout --spsProfiling (two trys).
Asking again for review here. Same patch as before, just a bit more of clean up.

This is working file locally ( and are created when using --spsProfile and that MOZ_UPLOAD_DIR is defined), I'll push to try with sps_profile later.
Attachment #8643314 - Attachment is obsolete: true
Attachment #8644844 - Flags: review?(jmaher)
Attachment #8644844 - Flags: review?(jmaher) → review+
Oh, I tried to build the syntax manually for spsProfile, seems like a failure. Canceled, and repushed to try (using trychooser this time):
Hmm, looks pretty good but I'm a bit worried with the two red jobs in:

Does not look related to my change to me, but maybe I should try again the job without my change to be sure ?
I would try again, it could have been a bad base you started with.
Joel, I have a bunch of errors that seems unrelated:

send failed, graph server says:
11:20:19     INFO -  RETURN:to determine geomean from 'test_run_values' for  50795710 - local variable 'values' referenced before assignment
11:20:19     INFO -  RETURN:  File "/var/www/html/graphs/server/pyfomatic/", line 295, in handleRequest
11:20:19     INFO -  RETURN:    geomean = valuesReader(databaseCursor, databaseModule, inputStream, metadata)
11:20:19     INFO -  RETURN:  File "/var/www/html/graphs/server/pyfomatic/", line 238, in valuesReader
11:20:19     INFO -  RETURN:    raise DatabaseException("to determine geomean from 'test_run_values' for  %s - %s" % (metadata.test_run_id, str(x)))
11:20:19     INFO -  RETURN:
11:20:19     INFO -  RETURN:

Also I still have two win8 talos chrome busted. Will retry those without my patch to be sure.
So tried without my patch, one busted, the other green. Will put back my patch an try again some more.
put pack my changes, on 4 jobs two busted, two success. It does not seems related to my change. I think we are good landing this, but maybe we should investigate what is this intermittent bug first ?

As you want Joel - I would go to land that patch, since this does not seems related an that it only appears when the spsProfile option is activated on try (I suspect I tried again on a bad base as you suggested). Then we should try again with spsProfile, and maybe fill a bug.
lets land the patch.  I suspect this is a specific test causing troubles.
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.