search

mahmoudimus / nose-timer

A timer plugin for nosetests (how much time does every test take?)
MIT License
126 stars 33 forks source link

Timer options #36

Closed skudriashev closed 5 years ago

skudriashev commented 10 years ago

Guys, currently I see a problem with using the --timer-ok and --timer-warning options. When the --timer-ok option is used it is expected that all tests which take less time than --timer-ok will be highlighted in green. The same with --timer-warning. But let's imagine somebody used the --timer-warning option only with value < 1s and expects to see all tests that took less time than --timer-warning in yellow, other - in red. But this will never happen, since all tests passed less then the --timer-ok (which is 1s by default) and they are marked as 'ok'. Yes, this a bit confusing.

I would propose to treat these option as thresholds of maximum pass time and use the --timer-error and --time-warning options. So the --timer-error will color all tests with red, if test took more time then specified threshold. The same with --time-warning. I think this is more natural (maybe I wrong).

Moreover, to prevent situations when these options intersect I think in case when at least one threshold specified - the default values should not be taken into account.

Folks, what are your thoughts?

mahmoudimus commented 9 years ago

@skudriashev this is actually a pretty great idea, but I wonder if it's going to break behavior for individuals who rely on this plugin. Most users will want this plugin to "just work" out of the box and maybe tweak one or two defaults.

This brings up this issue https://github.com/mahmoudimus/nose-timer/issues/7. Maybe we should develop a "filter-like" extensions that allows users to customize the instrumentation windows? Just thinking out loud, so take with a grain of salt.

skudriashev commented 9 years ago

@mahmoudimus yes, this would definitely break the current behavior. But, maybe this worth it? At least those who like current functionality can always install old release, can't they?

skudriashev commented 9 years ago

@mahmoudimus, @e0ne what do you think?

skudriashev commented 9 years ago

@mahmoudimus, @e0ne guys, please, take a look on this patch. You can try how this works from the user's prospective. IMHO it is much more clear how it works now. And this fixes bug I've described above.

skudriashev commented 9 years ago

@mahmoudimus, @e0ne any thoughts?

mahmoudimus commented 9 years ago

Hey @skudriashev - will take a look at this tonight!

skudriashev commented 9 years ago

@mahmoudimus Hi, any updates on this?:)

skudriashev commented 9 years ago

@mahmoudimus How are you?:) Any news from your side?

mahmoudimus commented 9 years ago

@skudriashev haven't forgotten about this, been a few busy days. hope you had a great new years!

skudriashev commented 9 years ago

@mahmoudimus hope you too :)

mahmoudimus commented 9 years ago

@skudriashev I think #66 and https://github.com/mahmoudimus/nose-timer/commit/72a3d5af71808d5d234ab051abde5ff7bd89055a are actually related to this issue (somewhat). Might be interesting for a v1.0 release if we do implement this option.

edit also, issue https://github.com/mahmoudimus/nose-timer/issues/7 is related.