Remove unnecessary attr_acessors in AV tests (and get rid of two method redefined warnings)

[utilum]

Fixes the following warnings when launching AV tests:

[oz@localhost actionview]$ bundle exec rake test
/home/oz/.rbenv/versions/2.3.3/bin/ruby -w -I"lib:test" -I"/home/oz/.rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/gems/rake-11.3.0/lib" "/home/oz/.rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/gems/rake-11.3.0/lib/rake/rake_test_loader.rb" "test/template/number_helper_test.rb" "test/template/form_helper/form_with_test.rb" "test/template/date_helper_i18n_test.rb" "test/template/form_helper_test.rb" "test/template/atom_feed_helper_test.rb" "test/template/output_safety_helper_test.rb" "test/template/controller_helper_test.rb" "test/template/digestor_test.rb" "test/template/dependency_tracker_test.rb" "test/template/sanitize_helper_test.rb" "test/template/lookup_context_test.rb" "test/template/text_test.rb" "test/template/text_helper_test.rb" "test/template/template_error_test.rb" "test/template/resolver_patterns_test.rb" "test/template/resolver_cache_test.rb" "test/template/streaming_render_test.rb" "test/template/html_test.rb" "test/template/record_identifier_test.rb" "test/template/render_test.rb" "test/template/active_model_helper_test.rb" "test/template/record_tag_helper_test.rb" "test/template/javascript_helper_test.rb" "test/template/url_helper_test.rb" "test/template/form_tag_helper_test.rb" "test/template/form_options_helper_i18n_test.rb" "test/template/translation_helper_test.rb" "test/template/test_test.rb" "test/template/template_test.rb" "test/template/tag_helper_test.rb" "test/template/test_case_test.rb" "test/template/erb_util_test.rb" "test/template/asset_tag_helper_test.rb" "test/template/form_options_helper_test.rb" "test/template/capture_helper_test.rb" "test/template/form_collections_helper_test.rb" "test/template/log_subscriber_test.rb" "test/template/erb/form_for_test.rb" "test/template/erb/tag_helper_test.rb" "test/template/partial_iteration_test.rb" "test/template/testing/fixture_resolver_test.rb" "test/template/testing/null_resolver_test.rb" "test/template/date_helper_test.rb" "test/template/compiled_templates_test.rb" 
Run options: --seed 28319

# Running:

....................S..................................................................................................................................................................................................................................................................................../home/oz/code/rails/actionview/test/template/form_helper_test.rb:1768: warning: method redefined; discarding old file
/home/oz/code/rails/actionview/test/template/form_helper_test.rb:1768: warning: method redefined; discarding old file=
..................................................................................../home/oz/code/rails/actionview/test/template/form_helper_test.rb:1754: warning: method redefined; discarding old file
/home/oz/code/rails/actionview/test/template/form_helper_test.rb:1754: warning: method redefined; discarding old file=
.....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

Finished in 13.763360s, 139.6461 runs/s, 311.9878 assertions/s.

1922 runs, 4294 assertions, 0 failures, 0 errors, 1 skips

You have skipped tests. Run with --verbose for details.

Removes two unnecessary Post.send calls from their respective tests.

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

[matthewd]

Can you add an explanation of why this is okay to do / why it doesn't change what the test is testing / why it was originally written this way?

[utilum]

Thank you.

I've traced the warnings back to #26976 (67f81cc), which added similar tests for form_with_file_field and fields_with_file_field.

Removing those two lines from actionview/test/template/form_helper/form_with_test.rb also makes the warnings go away. But all four may as well be eliminated.

The two tests in actionview/test/template/form_helper_test.rb, with their Post.send attr_accessr, :file and Comment.send attr_accessr, :file lines were created in b17b980 and 0523b55 towards Rails 3.1.
ActionView was still in ActionPack, inter-dependencies and all the changes made since are hard for me to untangle.

That said, currently, the removed lines have no impact on the the expected against which we assert.

The following passes:

  def test_post_send_has_no_impact_on_output_buffer
    keep_buffer = output_buffer
    Post.send :attr_accessor, :file
    assert_equal keep_buffer, output_buffer
    assert_equal "", output_buffer
  end

Removing all four makes the warnings go away and does not affect AV tests.

How to proceed from here?

[utilum]

@kaspth do you remember what purpose is served by the lines Post.send attr_accessor, :file and Comment.send attr_accessor, :file?

[kaspth]

@utilum Try finding out where the Post model is defined (and if it's already got a file accessor: just kill the line). Same goes for Comment.

[kaspth]

Looks like a rebase went wrong here too, so there's some unintended commits in here. Try rebasing them out 😉

[utilum]

Sorry, rebase cleaned. And thank you, @kaspth for having a look.
I'm not suggesting that Post or Comment already has a :file accessor, they do not. The redefinition (and the warnings) have only occurred after #26976, which redefines it.

If creating the accessor is needed, why do the tests pass without it?

Edit: The tests verify correctness of the generated form, which does not depend on the accessor (as proven by tests passing). Why keep the accessor creation after it starts giving warnings?

[kaspth]

They might pass just because of test order. Anyway, the models are in here, so just move the attr_accessor :file to there: https://github.com/rails/rails/blob/0343aafd06dc7234cecfe717ea5a0fcba01cbbfb/actionview/test/lib/controller/fake_models.rb

[kaspth]

Edit: The tests verify correctness of the generated form, which does not depend on the accessor (as proven by tests passing). Why keep the accessor creation after it starts giving warnings?

The accessor might just be defined elsewhere. I'd inspect with puts Post.new.method(:file).source_location in the test after the inline definition.

[utilum]

I'm not sure what has changed, but I can no longer replicate. Must have been solved :)

[prathamesh-sonpatki]

This was fixed in be984f0

[utilum]

Cool! I did notice that @amatsuda did a big cleanup. Just wondering why this identical PR was contested and left out.

[amatsuda]

@utilum Ugh, I'm so sorry I wasn't aware of the ongoing discussion here.
Maybe the tests are not actually accessing the :file field? I wasn't really sure, but I thought this must be a temporary code that the author just forgot to remove.

[utilum]

I am sorry @amatsuda , I did not mean it like that. This is what I meant:
@kaspth do you agree, now, that these accessors were unnecessary ?