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

Massive test refactorings. #24

Merged
merged 11 commits into from
Nov 24, 2011
Merged

Massive test refactorings. #24

merged 11 commits into from
Nov 24, 2011

Conversation

splattael
Copy link
Contributor

According to flay there was much duplication in test code. This isn't bad by default but it makes test_list.rb almost unreadable. At least for me.

With test/unit you can split up duplicate test code into modules. This killed many lines:

Before

  • test lines: 1355
  • flay score: 6268
  • rake: 71 tests, 439 assertions, 0 failures, 0 errors

After

  • test lines: 816
  • flay score: 2272
  • rake: 71 tests, 439 assertions, 0 failures, 0 errors

Future

flay still reports similiar code which can be refactored easily but I am not sure if you accept that huge pull request ;)

$ flay test/ | less
Total score (lower is better) = 2272


1) Similar code found in :defn (mass = 720)
  test/shared_list_sub.rb:7
  test/shared_list.rb:7
  test/shared_zero_based.rb:29
  test/shared_array_scope_list.rb:7

2) Similar code found in :defn (mass = 354)
  test/shared_list_sub.rb:83
  test/shared_list.rb:105
  test/shared_array_scope_list.rb:105

3) Similar code found in :defn (mass = 252)
  test/shared_list.rb:70
  test/shared_zero_based.rb:51

4) Similar code found in :defn (mass = 196)
  test/shared_list.rb:48
  test/shared_zero_based.rb:7

5) Similar code found in :defn (mass = 172)
  test/shared_list.rb:143
  test/shared_array_scope_list.rb:130

6) Similar code found in :defn (mass = 164)
  test/shared_list.rb:156
  test/shared_array_scope_list.rb:143

7) Similar code found in :defn (mass = 150)
  test/shared_list_sub.rb:29
  test/shared_list.rb:29
  test/shared_array_scope_list.rb:29

8) Similar code found in :defn (mass = 132)
  test/shared_list_sub.rb:35
  test/shared_list.rb:35
  test/shared_array_scope_list.rb:35

Please follow each commit if the result (Files Changed) is overwhelming.

If you have questions please drop a line :)

Peter Suschlik added 11 commits November 16, 2011 15:48
Pass options to define a default value for "pos" column.
Undefine method `default_test` to stop `test/unit`
from complaining about missing tests for that abstract test case.

before:
71 tests, 439 assertions, 0 failures, 0 errors

after:
71 tests, 439 assertions, 0 failures, 0 errors
This commit ensures that `default_position?` has a meaning
in `add_to_list_bottom` again.

Before that commit all tests would pass even if
you comment `|| default_position?`.
`flay -d test/` reported that ListTest and ListTestWithDefault
are almost identical (except the setup).

before:
71 tests, 439 assertions, 0 failures, 0 errors

after:
71 tests, 439 assertions, 0 failures, 0 errors
before:
71 tests, 439 assertions, 0 failures, 0 errors

after:
71 tests, 439 assertions, 0 failures, 0 errors
before:
71 tests, 439 assertions, 0 failures, 0 errors

after:
71 tests, 439 assertions, 0 failures, 0 errors
before:
71 tests, 439 assertions, 0 failures, 0 errors

after:
71 tests, 439 assertions, 0 failures, 0 errors
before:
71 tests, 439 assertions, 0 failures, 0 errors

after:
71 tests, 439 assertions, 0 failures, 0 errors
@swanandp
Copy link
Contributor

I am generally non-committal in refactoring tests. IMO, tests need not be very DRY. As long as they represent specs accurately, I am ok with it. I'll take some time review it if you don't mind.

@splattael
Copy link
Contributor Author

Sure, no problem.

You can cherry-pick commits if you like to. Note, there's also a bugfix at 0a054d6.

I've just realized that github displays commits in this pull request randomly.
Please follow https://github.com/neopoly/acts_as_list/commits/refactor_tests for the correct order.
First refactorings are pretty straightforward ;)

swanandp added a commit that referenced this pull request Nov 24, 2011
Massive test refactorings. This means we need to bump up the version as well.
@swanandp swanandp merged commit 7fd29b7 into brendon:master Nov 24, 2011
@swanandp
Copy link
Contributor

@splattael : Pulled! Thanks for contributing!

@swanandp
Copy link
Contributor

Gah, made the classic mistake of not running tests before merging.
@splattael: ./test/test_list.rb:69 is throwing an error Could not find table 'mixins'. Any chance you can look into it? Looks like a regression to me.

@splattael
Copy link
Contributor Author

Strange, the tests on my box pass (ree, 1.9.2 and 1.9.3).

Are you sure you were testing the merged code?

I had the same error during refactoring and fixed it here:
https://github.com/neopoly/acts_as_list/commit/ef5dc201568690570c99f5501fab5624156ef5f3#L0R70

@swanandp
Copy link
Contributor

Aye, I do have this commit merged, and I am on ree.

@splattael
Copy link
Contributor Author

Ok, thanks. Could please provide following information?

Ruby version

$ ruby -v
ruby 1.8.7 (2011-02-18 patchlevel 334) [x86_64-linux], MBARI 0x6770, Ruby Enterprise Edition 2011.03

Gem list

$ bundle exec gem list
activemodel (3.1.1)
activerecord (3.1.1)
activesupport (3.1.1)
acts_as_list (0.1.4)
arel (2.2.1)
builder (3.0.0)
bundler (1.0.21)
i18n (0.6.0)
json (1.6.1)
multi_json (1.0.3)
rdoc (3.11)
sqlite3 (1.3.4)
tzinfo (0.3.31)

rake output

$ rake
/home/ps/.rvm/rubies/ree-1.8.7-2011.03/bin/ruby -I"lib:lib:test" -I"/home/ps/.rvm/gems/ree-1.8.7-2011.03@global/gems/rake-0.9.2.2/lib" "/home/ps/.rvm/gems/ree-1.8.7-2011.03@global/gems/rake-0.9.2.2/lib/rake/rake_test_loader.rb" "test/**/test_*.rb" 
Loaded suite /home/ps/.rvm/gems/ree-1.8.7-2011.03@global/gems/rake-0.9.2.2/lib/rake/rake_test_loader
Started
.......................................................................
Finished in 0.947402 seconds.

71 tests, 439 assertions, 0 failures, 0 errors

Does someone else expierence that test failure?

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