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

EventProf Minitest integration #29

Merged

Conversation

@IDolgirev
Copy link
Contributor

@IDolgirev IDolgirev commented Sep 29, 2017

What is the purpose of this pull request?

It implements Minitest integration for EventProf (#21)

What changes did you make? (overview)

Custom Minitest plugin and reporter were added
Integration specs (based on the same Rspec ones) were added

Is there anything you'd like reviewers to focus on?

If there are no critical comments about I'll update EventProf guide

Copy link
Collaborator

@palkan palkan left a comment

Great job 👍

There are some small issues but overall looks good. Thank you!

Don't forget about:

  • Guide update

  • Changelog update

opts.on "--event-prof=EVENT", "Collects metrics for specified EVENT" do |event|
options[:event] = event
end
opts.on "--event-prof-rank=RANK_BY", "Defines RANK_BY parameter for results" do |rank|

This comment has been minimized.

@palkan

palkan Sep 29, 2017
Collaborator

Let's name it --event-prof-rank-by making more readable


def location(group, example)
name = group.public_instance_methods(false).find { |mtd| mtd.to_s == example }
group.instance_method(name).source_location

This comment has been minimized.

@palkan

palkan Sep 29, 2017
Collaborator

I've tried to run it with sample Rails app and got the following:

Run options: --event-prof=sql.active_record --seed 5773

# Running:

..../usr/local/lib/ruby/gems/2.4.0/bundler/gems/test-prof-3b646bac1b96/lib/test_prof/event_prof/minitest.rb:60:in `instance_method': nil is not a symbol nor a string (TypeError)
	from /usr/local/lib/ruby/gems/2.4.0/bundler/gems/test-prof-3b646bac1b96/lib/test_prof/event_prof/minitest.rb:60:in `location'
	from /usr/local/lib/ruby/gems/2.4.0/bundler/gems/test-prof-3b646bac1b96/lib/test_prof/event_prof/minitest.rb:52:in `change_current_group'
	from /usr/local/lib/ruby/gems/2.4.0/bundler/gems/test-prof-3b646bac1b96/lib/test_prof/event_prof/minitest.rb:38:in `track_current_example'
	from /usr/local/lib/ruby/gems/2.4.0/bundler/gems/test-prof-3b646bac1b96/lib/test_prof/event_prof/minitest.rb:19:in `prerecord'

I think, this is something Rails specific but we should care.

Example app is here https://github.com/palkan/th-dummy/tree/test-prof-minitest

This comment has been minimized.

@palkan

palkan Sep 29, 2017
Collaborator

Also, FYI rails/rails#29469 (maybe, it worth mentioning this issue in the guide).

@IDolgirev
Copy link
Contributor Author

@IDolgirev IDolgirev commented Sep 30, 2017

Thanks for the review!
I've already fixed that issue, but found one more little bug I want to fix before update the PR.
Do you have some suggestions about version in changelog?

@palkan
Copy link
Collaborator

@palkan palkan commented Sep 30, 2017

@IDolgirev

Do you have some suggestions about version in changelog?

Let's use "master"; I'll update it before releasing.

@IDolgirev
Copy link
Contributor Author

@IDolgirev IDolgirev commented Sep 30, 2017

Thanks, got it!

@@ -13,7 +13,7 @@ def log(level, msg)
end

def build_log_msg(level, msg)
colorize(level, "[TEST PROF #{level.to_s.upcase}] #{msg}")
colorize(level, "\n[TEST PROF #{level.to_s.upcase}] #{msg}")

This comment has been minimized.

@palkan

palkan Oct 2, 2017
Collaborator

What's the reason for the change? I think, it's better to control newlines in-place and do not hide within a private method.

This comment has been minimized.

@IDolgirev

IDolgirev Oct 2, 2017
Author Contributor

Yep, you're right - it was just a workaround for default Minitest reporter, where progress line concatenates with EventProf message. I'll remove it.

@@ -37,7 +38,7 @@ def after_test(*); end
def report
@profiler.group_finished(@current_group)
result = @formatter.prepare_results

puts "\n"

This comment has been minimized.

@palkan

palkan Oct 2, 2017
Collaborator

Bad idea. We should output everything using log, 'cause (in theory) it can write to file.
Just prepend the result with "\n" (i.e. log :info, "\n#{result}").

This comment has been minimized.

@IDolgirev

IDolgirev Oct 2, 2017
Author Contributor

Actually, it won't help with the concatenation issue, because we need new line before log message itself, but not before the results. But as I wrote below I agree that it was a dumb mistake to didn't keep in mind a possibility to write results to the file :)

@IDolgirev
Copy link
Contributor Author

@IDolgirev IDolgirev commented Oct 2, 2017

Frankly, I don't know what I was thinking about at that moment. Sorry for such a silly mistake :(

Also there is one more thing I need to mention - even all specs are green I think that EventProf guide need to be updated a little bit with some caveat about minitest-reporters. When the user unintentionally run specs without prepending them with bundle exec, he (or she) will get an error, because Minitest initialize all its plugins from minitest/*_plugin.rb of the every entry in the $LOAD_PATH, and minitest-reporters as just installed gem is available there. And when this gem is not properly configured as I mentioned in the guide, some hooks around the test are become unavailable which causes the error.
At the moment I suggest to update the guide with that, but will try to investigate it deeper. WDYT?

@palkan
Copy link
Collaborator

@palkan palkan commented Oct 2, 2017

At the moment I suggest to update the guide with that, but will try to investigate it deeper

Sounds good!

@palkan
palkan approved these changes Oct 2, 2017
@palkan palkan merged commit 9e4152a into test-prof:master Oct 2, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@palkan palkan mentioned this pull request Oct 2, 2017
@palkan
Copy link
Collaborator

@palkan palkan commented Oct 2, 2017

@IDolgirev Thank you for your help! I need your full name in Russian for CultOfMartians and your Twitter nickname (if any) for the announcement.

@IDolgirev
Copy link
Contributor Author

@IDolgirev IDolgirev commented Oct 2, 2017

You're welcome! Glad to help :)
My name in Russian is Илья Долгирев and Twitter nickname is @Dolgirev

@palkan
Copy link
Collaborator

@palkan palkan commented Oct 2, 2017

@IDolgirev Take a look, please: wrong event count https://travis-ci.org/palkan/test-prof/jobs/282329413

@IDolgirev
Copy link
Contributor Author

@IDolgirev IDolgirev commented Oct 2, 2017

Thanks, already trying to reproduce it locally.

As you see the whole group is missing in the results.

I'm not quite good at multithreading and probably need some advices but at the first glance the reason is inside report hook when the notification to finish last group is send to profiler it should be handled first, and only then we can use results.
Is it possible to get wrong order of execution of lines inside the same method due to the nature of JRuby?

@palkan
Copy link
Collaborator

@palkan palkan commented Oct 2, 2017

Is it possible to get wrong order of execution of lines inside the same method due to the nature of JRuby?

Yep, that may be the case. Not sure, but maybe Minitest automatically runs in multithreaded mode in JRuby.

@IDolgirev
Copy link
Contributor Author

@IDolgirev IDolgirev commented Oct 3, 2017

Well, I was totally unlucky yesterday so I didn't even get a chance to reproduce the error. Single spec, whole suite, random order, specific seed from Travis' failed job - nothing :(
Maybe you have some tricks how to deal with such type of errors?

@palkan
Copy link
Collaborator

@palkan palkan commented Oct 3, 2017

Have you tried JRuby?)

Let's not spend too much time; I will try to investigate the next this problem occur.

Thank you!

@IDolgirev
Copy link
Contributor Author

@IDolgirev IDolgirev commented Oct 3, 2017

In that case it was definitely JRuby because my machine started to work like any Dyson devices :)
And yes, I've checked it via htop :)
OK, feel free to ping me if you need some help with that next time.

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

Successfully merging this pull request may close these issues.

None yet

2 participants