EventProf Minitest integration #29

IDolgirev · 2017-09-29T06:24:54Z

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

…mples by count

…for factory.create

palkan

Great job 👍

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

Don't forget about:

Guide update
Changelog update

palkan · 2017-09-29T13:20:21Z

lib/minitest/event_prof_plugin.rb

+    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|


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

palkan · 2017-09-29T14:08:35Z

lib/test_prof/event_prof/minitest.rb

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


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

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

IDolgirev · 2017-09-30T17:54:19Z

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 · 2017-09-30T18:16:10Z

@IDolgirev

Do you have some suggestions about version in changelog?

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

IDolgirev · 2017-09-30T18:18:00Z

Thanks, got it!

palkan · 2017-10-02T11:43:08Z

lib/test_prof/logging.rb

@@ -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}")


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.

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

palkan · 2017-10-02T15:34:50Z

lib/test_prof/event_prof/minitest.rb

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


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}").

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 · 2017-10-02T16:18:12Z

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 · 2017-10-02T16:37:05Z

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

Sounds good!

palkan · 2017-10-02T16:39:58Z

@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 · 2017-10-02T16:43:33Z

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

palkan · 2017-10-02T17:04:56Z

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

IDolgirev · 2017-10-02T18:20:17Z

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 · 2017-10-02T19:23:50Z

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 · 2017-10-03T06:59:32Z

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 · 2017-10-03T07:29:42Z

Have you tried JRuby?)

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

Thank you!

IDolgirev · 2017-10-03T07:44:22Z

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.

IDolgirev added 11 commits September 28, 2017 10:39

Add integration spec and related fixture for Minitest

12d26b9

Add EventProf Minitest plugin

37ecdcb

Switch from Minitest::Reporters to Minitest

728f4eb

Add EventProf Minitest reporter

a2aa629

Add results formatter for EventProf Minitest reporter

8287c58

Move formatter to proper namespace

199f573

Extend fixture and fix spec to avoid random failures with ranking exa…

c9d4cee

…mples by count

Simplify spec structure. Add CustomEvents context and proper fixture …

c8b174a

…for factory.create

Expand spec with Sidekiq custom event and add fixture

973721a

Add more configuration options to Minitest plugin

ea92518

Fix Rubocop warnings

3b646ba

palkan requested changes Sep 29, 2017

View reviewed changes

IDolgirev added 4 commits October 2, 2017 12:28

Fix concatenation of progress line and EventProf results

bffd001

Fix issue with nested groups. Add support for Minitest::Reporters

6c41c61

Update EventProf guide

7f68abd

Merge branch 'master' into feature_add_event_prof_minitest_integration

45f2c1a

palkan requested changes Oct 2, 2017

View reviewed changes

IDolgirev added 2 commits October 2, 2017 16:37

Fix wrong desision with new line

dcbea9a

Fix Rubocop warning about complexity. Add required new line in place

ba523eb

palkan reviewed Oct 2, 2017

View reviewed changes

palkan approved these changes Oct 2, 2017

View reviewed changes

palkan merged commit 9e4152a into test-prof:master Oct 2, 2017

palkan mentioned this pull request Oct 2, 2017

EventProf Minitest support #21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EventProf Minitest integration #29

EventProf Minitest integration #29

IDolgirev commented Sep 29, 2017

palkan left a comment

palkan Sep 29, 2017

palkan Sep 29, 2017

palkan Sep 29, 2017

IDolgirev commented Sep 30, 2017

palkan commented Sep 30, 2017

IDolgirev commented Sep 30, 2017

palkan Oct 2, 2017

IDolgirev Oct 2, 2017

palkan Oct 2, 2017

IDolgirev Oct 2, 2017

IDolgirev commented Oct 2, 2017

palkan commented Oct 2, 2017

palkan commented Oct 2, 2017

IDolgirev commented Oct 2, 2017

palkan commented Oct 2, 2017

IDolgirev commented Oct 2, 2017

palkan commented Oct 2, 2017

IDolgirev commented Oct 3, 2017

palkan commented Oct 3, 2017

IDolgirev commented Oct 3, 2017

EventProf Minitest integration #29

EventProf Minitest integration #29

Conversation

IDolgirev commented Sep 29, 2017

What is the purpose of this pull request?

What changes did you make? (overview)

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

palkan left a comment

Choose a reason for hiding this comment

palkan Sep 29, 2017

Choose a reason for hiding this comment

palkan Sep 29, 2017

Choose a reason for hiding this comment

palkan Sep 29, 2017

Choose a reason for hiding this comment

IDolgirev commented Sep 30, 2017

palkan commented Sep 30, 2017

IDolgirev commented Sep 30, 2017

palkan Oct 2, 2017

Choose a reason for hiding this comment

IDolgirev Oct 2, 2017

Choose a reason for hiding this comment

palkan Oct 2, 2017

Choose a reason for hiding this comment

IDolgirev Oct 2, 2017

Choose a reason for hiding this comment

IDolgirev commented Oct 2, 2017

palkan commented Oct 2, 2017

palkan commented Oct 2, 2017

IDolgirev commented Oct 2, 2017

palkan commented Oct 2, 2017

IDolgirev commented Oct 2, 2017

palkan commented Oct 2, 2017

IDolgirev commented Oct 3, 2017

palkan commented Oct 3, 2017

IDolgirev commented Oct 3, 2017