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

[WIP] Test Updates: Part 2 of 3 #1860

Closed

Conversation

NullVoxPopuli
Copy link
Contributor

Purpose

Using morre proper test naming

Changes

find def test_(.+)$
replace with test '$1' do

also, replace

def my_method

ensure

end

with

test 'my_method'
  begin
  ensure
  end
end

Caveats

ensure can no longer be used by itself, as test blocks aren't methods

Related GitHub issues

#1850

Additional helpful information

Hopefully this will encourage future contributions to use the same test style

@NullVoxPopuli
Copy link
Contributor Author

current issue: test is being interpreted weird, and it thinks the string is a command

@NullVoxPopuli
Copy link
Contributor Author

Interesting: I can't find much mention of the test 'what am I testing' syntax, but there is a lot of talk of minitset supporting expectation style test naming... hmm

@NullVoxPopuli
Copy link
Contributor Author

I may put this on hold, and skip to step 3 (which is just re-organizing the test files)

@@ -22,12 +22,12 @@ def render_skipping_adapter

tests AdapterSelectorTestController

def test_render_using_default_adapter
test 'render_using_default_adapter' do
Copy link
Member

Choose a reason for hiding this comment

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

this syntax comes from ActiveSupport::TestCase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

@@ -42,6 +42,7 @@ group :bench do
end

group :test do
gem 'minitest'
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 in the gemspec. Was something not working?

@bf4
Copy link
Member

bf4 commented Aug 1, 2016

Looks fine to me once tests pass. Maybe leave all the ones with ensure alone for now?

@NullVoxPopuli
Copy link
Contributor Author

I'm going to re-do this PR with your suggestions after #1863 is merged (hopefully). It's mostly a regex replace on the test names. And now that I know about ActiveSupport::Test, everything should pass next attempt

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.

2 participants