Closed Bug 653891 Opened 11 years ago Closed 10 years ago

remove running display of elapsed time

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: KWierso, Assigned: KWierso)

Details

Attachments

(3 files)

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.
Attachment #529259 - Flags: review?(myk)
Alternatively, just remove the running elapsed time indicator completely.

:)
Attachment #529260 - Flags: review?(myk)
Remove it completely or pref it off, Brian: what say you?
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).
(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.
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?
> 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 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.
Attachment #529260 - Flags: review?(myk) → review+
Attachment #529259 - Flags: review?(myk)
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.
I'll write up a patch for that.
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
Attachment #529571 - Flags: review?(myk) → review+
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.
No longer blocks: addonsdk10b5
Summary: Make running display of elapsed time opt-in → remove running display of elapsed time
Assignee: nobody → myk
Priority: -- → P1
Target Milestone: --- → 1.0
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.
Assignee: myk → kwierso
OS: Windows 7 → All
Hardware: x86 → All
Assignee: kwierso → rFobic
er - reassigning to Wes as per Myk's last comment. Will put on Myk's deliverables list on wiki however
Assignee: rFobic → kwierso
Brian: are we ready to remove this, or should we leave it in for 1.0 and beyond?
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.
Landed, in https://github.com/mozilla/addon-sdk/commit/af594fb75ed6d864842b0525149e5da6c1e8fca7 .
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.