-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
Remove unnecessary attr_acessors in AV tests (and get rid of two method redefined warnings) #27307
Conversation
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. |
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? |
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 The two tests in actionview/test/template/form_helper_test.rb, with their That said, currently, the removed lines have no impact on the the expected against which we assert. The following passes:
Removing all four makes the warnings go away and does not affect AV tests. How to proceed from here? |
@kaspth do you remember what purpose is served by the lines |
@utilum Try finding out where the |
Looks like a rebase went wrong here too, so there's some unintended commits in here. Try rebasing them out 😉 |
Sorry, rebase cleaned. And thank you, @kaspth for having a look. 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? |
71f4f17
to
0343aaf
Compare
They might pass just because of test order. Anyway, the models are in here, so just move the |
The accessor might just be defined elsewhere. I'd inspect with |
I'm not sure what has changed, but I can no longer replicate. Must have been solved :) |
This was fixed in be984f0 |
Cool! I did notice that @amatsuda did a big cleanup. Just wondering why this identical PR was contested and left out. |
@utilum Ugh, I'm so sorry I wasn't aware of the ongoing discussion here. |
Fixes the following warnings when launching AV tests:
Removes two unnecessary Post.send calls from their respective tests.