Skip to content
This repository was archived by the owner on Nov 30, 2024. It is now read-only.

WIP Refactor group profile to include beforehook #1942

Closed
wants to merge 18 commits into from

Conversation

denislaliberte
Copy link
Contributor

@JonRowe after your comments of yesterday I have done some cleanup, If you have other comments or hint to fix json_formatter_spec or profile_groupe.feature let me know

The json_formatter_spec change the clock of the example, do you think i should rewrite "ranks the example groups by average time" with it's own 'before' or test this behaviour in the new profile_groupe.feature ?

issue #1878

…formatter_spec, remove test_spec and fix profile_groupe.feature
@denislaliberte
Copy link
Contributor Author

I noticed the to_h function in ruby < 2.1, it's on my todo.

@JonRowe you can see the problem with the profile_groupe.feature on the ruby 2.2 build

@@ -634,6 +631,7 @@ class NullNotification
end

# `CustomNotification` is used when sending custom events to formatters /

Copy link
Member

Choose a reason for hiding this comment

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

This is superfluous and should be removed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, no problem :)

end
"""
When I run `rspec spec --profile`
Then the output should contain "slow before context hook"
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 the examples in this scenario are kind of noisy since there are so many of them. IMO, it would work better as documentation so that the reader doesn't have to read so much to get the full context. You can use rspec spec --profile <number> to limit it to the top <number> slowest examples.

I also think we can cut the sleep values further...maybe sleep 0.1 and sleep 0.2? It's nice to keep the scenario from getting too slow. We can always disable GC in it to make the runtime more deterministic.

next if location_hash.key?(:description)
location_hash[:description] = example.example_group.top_level_description
end
# todo rename this method or move it's code to the json_formater.rb and profile.formater.rb
Copy link
Member

Choose a reason for hiding this comment

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

This code shouldn't be moved to the formatters, it should be de-duplicated here.

@myronmarston
Copy link
Member

@denislaliberte -- thanks for taking this PR this far! @JonRowe, do you want to work on taking this across the finish line so it can be in 3.3?

@JonRowe
Copy link
Member

JonRowe commented May 22, 2015

Yep sure

@denislaliberte
Copy link
Contributor Author

@myronmarston no problem, It was fun to give a try on this issue but in the end I was stuck, I can't add example_group_started on the notifications class without a larger refactoring...

I will look to other issue, I'll be glad to contribute if I can.

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

Successfully merging this pull request may close these issues.

3 participants