653891 - remove running display of elapsed time

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: add a "--timer" argument to cfx run and cfx test

Spamming the console every 5 seconds is bad in the general case, although it could be helpful for figuring out the test failures.

Make it so you have to opt-in to the spam.

This patch adds a command line flag "--timer" (or "-t" shorthand), which turns on the spam.

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Just remove elapsed time

Alternatively, just remove the running elapsed time indicator completely.

:)

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Remove it completely or pref it off, Brian: what say you?

Comment 3

[Brian Warner [:warner :bwarner]]

What about removing it completely for the release branch, but leaving it in for trunk? I know it's annoying.. are you using the released SDK, or trunk from git? (The buildbot always runs from trunk, at least for now).

Comment 4

[Matěj Cepl]

(In reply to comment #3)
> What about removing it completely for the release branch, but leaving it in for
> trunk? I know it's annoying.. are you using the released SDK, or trunk from
> git? (The buildbot always runs from trunk, at least for now).

Please, don't. There are people (like me) who are running trunk on daily basis.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Yeah, I think the behavior should be consistent no matter which build you're coming from.

Also, I'm not quite sure how you'd get it to know that it's running from a released version.

IMO, the first patch is the way to go if the running times are still wanted.

As another alternative, would it be possible to get the running times to show only for cfx test, and not cfx run?

Comment 6

[Brian Warner [:warner :bwarner]]

> Also, I'm not quite sure how you'd get it to know that it's running from a
> released version.

I'm talking about applying a remove-the-display patch to the b5 branch, but
not applying that patch to trunk. (I certainly don't want the code to switch
on its own version number or anything crazy like that).

I think we need the display on the buildbot for another couple of weeks,
until we're sure that we've tracked down all the hangs. Let's say if we get
four weeks of successful non-hanging tests, then we'll stop using it on the
buildbot.

I don't think we ever need the display on released versions: I don't want to
ship a b5 or a 1.0 with the display.

We could add a switch, but then we'd need to change the buildbot's script
(specifically the "run_jetpack.sh" shell script in the "build-tools"
repository) to add the switch while running the buildbot. We could then
remove both the switch and the run_jetpack.sh code which adds it in four
weeks. I don't really want to be changing the code all that much, especially
for a tiny little debugging message that will be gone in a month, so I admit
that I'd feel kinda silly adding a --show-elapsed-time argv switch.

Yeah, we could change it to only show the time when running 'cfx test', by
adding an extra argument into the internal runner() call. That might not
trigger my feeling-kinda-silly sense quite so badly :).

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 529260 [details] [diff] [review]
Just remove elapsed time

r,a=myk for the 1.0b5 branch.

For the trunk, let's do as Brian suggests and limit it to |cfx test|.  Then, once we have a handle on the test hangs, let's remove it completely.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Landed the removal patch on the 1.0b5 branch:

https://github.com/mozilla/addon-sdk/commit/c373d0e6a465d176ca6d13b2d032cc6bbb1c3aa7

Leaving bug open for the |cfx test|-only trunk fix.

Comment 9

[Brian Warner [:warner :bwarner]]

I'll write up a patch for that.

Comment 10

[Brian Warner [:warner :bwarner]]

Attachment: only emit elapsed time in 'cfx test', not 'cfx run'

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 529571 [details] [diff] [review]
only emit elapsed time in 'cfx test', not 'cfx run'

Looks good, works well, bonus points for keeping --use-server working! r=myk

Comment 12

[Brian Warner [:warner :bwarner]]

landed in https://github.com/mozilla/addon-sdk/commit/34e643eb06b6030426513c7ddee9e696399f1b6f

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Morphing this bug to be about removing the output of elapsed time once it has served its purpose.  And removing block on 1.0b5, since the blocker issue has been resolved.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Erm, actually this is more properly assigned to Wes, who submitted the patch to fix it, even if I'm going to be the one checking it in.  Fixing.

Comment 15

[David Mason [:dcm :dmason]]

er - reassigning to Wes as per Myk's last comment. Will put on Myk's deliverables list on wiki however

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Brian: are we ready to remove this, or should we leave it in for 1.0 and beyond?

Comment 17

[Brian Warner [:warner :bwarner]]

I guess we can remove it. There's a chance that removing it will make analyzing buildbot test failures a little bit harder, but I think we've resolved all the hang-failures for now.

Comment 18

[Brian Warner [:warner :bwarner]]

Landed, in https://github.com/mozilla/addon-sdk/commit/af594fb75ed6d864842b0525149e5da6c1e8fca7 .