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

Address NameError on triple nested sub_test_case under ActionView::TestCase on Rails 6.1 #24

Merged
merged 1 commit into from
Dec 20, 2021

Conversation

amatsuda
Copy link
Member

@amatsuda amatsuda commented Dec 19, 2021

Since Rails 6.1 (Zeitwerk), all ActionView::TestCase class is expected to abide by normal Ruby module naming, but this is not always the case with sub_test_case since sub_test_case generates an anonymous class with the given string as its name.

Attached is a simple reproduction of the situation, and a monkey-patch for ActionView::TestCase's sub_test_case to ignore NameError from default_helper_module!.
It is less likely that this patch introduces any new unexpected bug because that's basically how things were working with older versions of Rails with test-unit.

Please note that this patch depends on test-unit#207. We need a patched version of test-unit to be bundled to this repo in order for this patch to run (and so I expect that that test I added here would raise errors on the CI for this PR).

amatsuda added a commit to kaminari/kaminari that referenced this pull request Dec 19, 2021
@kou
Copy link
Member

kou commented Dec 19, 2021

Thanks.

It seems that we can avoid the default_helper_module! call by defining a custom anonymous? method:

https://github.com/rails/rails/blob/v6.1.4.4/actionpack/lib/abstract_controller/helpers.rb#L51

    def sub_test_case_class(*)
      klass = super
      class << klass
        define_method :anonymous? do
          true
        end
      end
      klass
    end

Which approach is better from a Rails developer view?

(I'm OK with both this pull request approach and the anonymous? approach.)

…stCase on Rails 6.1

Since Rails 6.1+ (Zeitwerk), all ActionView::TestCase class is expected to abide
by normal Ruby module naming, but this is not always the case with sub_test_case
since sub_test_case generates an anonymous class with the given string as its name.
@amatsuda
Copy link
Member Author

It seems that we can avoid the default_helper_module! call by defining a custom anonymous? method:

Thank you for the suggestion!
I knew this but at first I chose not to change the behavior of anonymous? because I felt that claiming something with a name as an anonymous object is simply a falsely claim, and so it may cause some unexpected side effect.
But I changed my mind after some more investigation after seeing your comment, because this Module#anonymous? definition is actually not in use anywhere else than this part.

Now that I pushed the change and seeing that all tests are passing, could you review this again?

@kou kou merged commit 18bde39 into test-unit:master Dec 20, 2021
@kou
Copy link
Member

kou commented Dec 20, 2021

Thanks for considering these approaches.
I've merged and released a new version.

@amatsuda amatsuda deleted the sub_test_case_name_error branch December 20, 2021 11:37
@amatsuda
Copy link
Member Author

@kou Thank you very much for the quick merge and release!

amatsuda added a commit to kaminari/kaminari that referenced this pull request Dec 20, 2021
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.

None yet

2 participants