Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config option to specify profile example count #747

Merged
merged 10 commits into from
Dec 24, 2012

Conversation

greggroth
Copy link

This adds a configuration option to specify the number of examples to include in the profile output.

addresses #726

@dchelimsky
Copy link
Contributor

@greggroth almost all of the RSpec.configuration options are also available on the CLI. @myronmarston do you agree that this should be in both places?

@soulcutter
Copy link
Member

I actually think this makes sense NOT having a command line option, the reasoning being that it depends on another option being set.

@greggroth
Copy link
Author

@soulcutter rspec --profile enables the profile, so it would make sense to me that rspec --profile 2 would show the profile with 2 examples.

@dchelimsky
Copy link
Contributor

@soulcutter I think it's more common to use --profile on the CLI because it's something you don't want to see every time (too much noise for TDD cycle w/ autotest/guard/etc).

How about, instead of adding a separate option, we allow --profile to accept a number:

rspec --profile 200

And in RSpec.configure we can support:

RSpec.configure {|c| c.profile_examples = 200}

@soulcutter
Copy link
Member

@greggroth That's not too bad, I guess. I guess I was worried more about something like rspec --profile --profile-count 2 . I didn't think about this too long, clearly!

@dchelimsky You're right :)

@greggroth
Copy link
Author

@dchelimsky I hadn't thought of it like that. Changing profile_examples from boolean to truthey/falsey should still work for whether it shows the profile and then I can pull the number from it (or if it's just true, make it 10). Do you like that more than two separate options?

@dchelimsky
Copy link
Contributor

@greggroth yes.

@greggroth
Copy link
Author

👍 I'm on it.

describe "something" do
it "foos take longest" do
container = Array.new
100_000.times { |n| container << n }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to see an explicit slowdown (e.g. using sleep) rather than an implicit one using a loop like this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if sleep would be a problem since it would slow down the test suite. I'll give it a shot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing something 100,000 times slows down the test suite, too. sleep is just less obtuse and more explicit.

Just make sure to use a low value (e.g. sleep 0.1).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to mention the memory... good catch

@greggroth
Copy link
Author

Can one of you check out the above two commits and let me know if I went about it the right way? Thanks!

@greggroth
Copy link
Author

I'm also aware of the test failures and I'm looking into those now.

it "eleventh example" do
sleep 0.05
11.should == 11
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a little confusing that you've got some examples named "foos/bazzes/bars take X" and others are named "nth example". It'd be nice to have a consistent naming scheme here. Maybe something like it "sleeps for X seconds (example #Y)"? (where X and Y would be filled in for the specific example)

@myronmarston
Copy link
Member

This is looking good. One general question: should setting RSpec.configure.profile_examples = 20 always cause profiling output to be dumped, or only if the --profile flag was set? I can see a couple different ways people will want to use this:

  • Some will want to set a particular number to be dumped, but they won't want the output every time. They'll want the profile to be dumped if and only if the --profile flag has been passed.
  • Others will want it always to be dumped, and may interpret the profile_examples setting as turning on profiling.

For me personally, my workflow is more in line with the first: in my minute-by-minute TDD cycle, I run individual spec files one-off, and I don't want the profile output for that. Once I'm ready to commit and/or push, I run the entire suite, and I do want the profile output then. I kinda wonder if those semantics would be confusing, though, so it might be better to stick with the second.

@sferik -- I'm curious on your thoughts here, given that you were the original requestor of this feature.

Greggory Rothmeier added 3 commits December 12, 2012 12:55
* fix fragile spec which depends on line distance from where the spec is
defined.
@sferik
Copy link

sferik commented Dec 13, 2012

I'm typically in one of three modes when doing software development:

  1. Normal Mode, where I'm writing code, writing specs, and refactoring.
  2. Debugging Mode, where there is a specific problem I'm trying to reproduce and identify.
  3. Optimization Mode, where I am benchmarking variations on existing code and design.

I use a different set of tools and flags in each of these modes. For example, I typically (Normal Mode) run rspec with the --fail-fast flag enabled, because I expect all specs to pass and I want to know as soon as one doesn't. Whereas, if I'm I debugging an issue that's causing many specs to fail, I won't use --fail-fast because I want to know if I'm making progress.

I also typically (Normal Mode) run specs with the --profile flag enabled to ensure that the new specs I'm writing are not unusually slow. I also use the --profile flag in Optimization Mode, especially when I'm trying to optimize the performance of my specs. However, after I optimize the slowest 10 specs as much as I can, they may still be the 10 slowest. I want the ability to see the 11th to 20th slowest specs, so I can try to optimize them after the first 10 are optimized.

This is a long way of saying, I'd like it to be easy for me to switch between these different modes.

In my opinion, setting RSpec.configure {|c| c.profile_examples = 200} should have the same behavior as setting the --profile=200 flag. I think it's confusing if the command-line flags behave differently from the configuration options. If you want to run an individual spec file without profiling, you should be able to pass the --no-profile flag on the command-line, which should override the configuration block.

@greggroth
Copy link
Author

I agree with @sferik. I like the idea of a --no-profile flag that overrules the config option. I'll add that to this PR.

@myronmarston
Copy link
Member

Sounds good.

@greggroth
Copy link
Author

@myronmarston this is ready for review when you have a chance. Thanks!

@@ -506,6 +508,19 @@ def reporter
end
end

# @api private
#
# Defaults `profile_examples` to 10 examples when `@profile_exmaples` is `true`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/exmaples/examples/

@myronmarston
Copy link
Member

@greggroth -- thanks for letting me know. I left a couple more comments.

@myronmarston
Copy link
Member

@greggroth -- do you want to take care of addressing my last couple comments before I merge? I can certainly take care of them but I always like to give the chance to contributors to address things first.

@greggroth
Copy link
Author

I'll take care of this tomorrow. Sorry I've been slow -- holidays and all. ;-)

Thanks!

On Sunday, December 23, 2012 at 8:06 PM, Myron Marston wrote:

@greggroth (https://github.com/greggroth) -- do you want to take care of addressing my last couple comments before I merge? I can certainly take care of them but I always like to give the chance to contributors to address things first.


Reply to this email directly or view it on GitHub (#747 (comment)).

@greggroth
Copy link
Author

Sorry for the delay. Those two commits should address all of your comments.

myronmarston added a commit that referenced this pull request Dec 24, 2012
config option to specify profile example count
@myronmarston myronmarston merged commit 2115f88 into rspec:master Dec 24, 2012
@myronmarston
Copy link
Member

Thanks @greggroth -- this looks great.

And no need to apologize...you do this in your free time like the rest of us :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants