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

Rubocop legacy fix: Erroneous Auto Inheritance of ActionDispatch::IntegrationTest in the World #505

Closed
luke-hill opened this issue Mar 31, 2021 · 5 comments
Labels
🏦 debt Tech debt 🔧 build Related to build / release process

Comments

@luke-hill
Copy link
Contributor

Summary

We inherit from ActionDispatch::IntegrationTest which inherits from about 4 other things down the chain. We need to try remedy this and only inherit what we need (Which are the test helpers).

@luke-hill luke-hill changed the title Bug / stylistic tweak Rubocop legacy fix: Erroneous Auto Inheritance of ActionDispatch::IntegrationTest in the World Apr 16, 2021
@aslakhellesoy aslakhellesoy added 🏦 debt Tech debt 🔧 build Related to build / release process labels Apr 26, 2021
@BrianHawley
Copy link
Contributor

BrianHawley commented Feb 15, 2022

I've been using the fact that World inherits from ActionDispatch::IntegrationTest in a patch that actually enables proper Rails transaction-mode database cleanup with no database_cleaner required at all. Been using this patch for more than a year with no issues. I was going to make a PR to upstream the patch into cucumber-rails itself. If you no longer inherit from ActionDispatch::IntegrationTest, Rails transaction-mode database cleanup won't work.

Just saying: It's not a bug that World inherits from ActionDispatch::IntegrationTest. It's a bug that it doesn't take advantage of the lifecycle code available in that class.

@luke-hill
Copy link
Contributor Author

Thanks for the heads up. It's likely we need to fix this in a breaking change release.

The reason I made this ticket was because we can streamline what rails should be and the inheritance (at that point), was only to get some test helpers (nothing more).

Maybe in doing so we find that we need more of the inheritance chain. I don't know. But definitely good you've posted this so I can be careful as/when I tackle this.

@luke-hill
Copy link
Contributor Author

luke-hill commented Oct 9, 2023

@BrianHawley - As a heads up. I'm going to try a small tweak, to just ensure that the class is setup correctly by calling super on the existing initialize method. This will alleviate rubocop and give us something to learn from - if this also causes issues then I think we might have to put more guards around any modifications here

Now this "could" be potentially problematic. So for now I'm just going to push the branch up and invite a few people to play with it.

EDIT: branch ref is v3/version_deps - It will enable cucumber v9.x it will also update rubocop for dev (Not visible in running), and do a slight tweak to the initializer

@luke-hill
Copy link
Contributor Author

As an update this branch is merged into main

I dug into some of the code and found the initializer was a "hack" set up 13 years ago. With the comment "Remove this". So after checking and removing it. I found that 4 levels deep the super call will require a name property. So I just added in a super call with a name and then from there everything else worked fine.

This obviously doesn't fix the issue of inheriting from it, but when I removed that I found a whole word of hurt happened including things like standard rails methods (model_path for example), started failing.

As such I'll keep this issue open for a couple of weeks, but then I'll close it off as no longer relevant.

@luke-hill
Copy link
Contributor Author

Closing as no longer relevant. cc/ @BrianHawley

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏦 debt Tech debt 🔧 build Related to build / release process
Projects
None yet
Development

No branches or pull requests

3 participants