-
Notifications
You must be signed in to change notification settings - Fork 137
cleanup support for Rails 2.3 and remove old integration tests #280
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
Conversation
I must admit that I didn’t understand this stuff, however I wondered whether rather than deleting it, it needed to be adapted to rails 6.1 and 7.x to actually run specific rails tests there - which is why I didn’t delete them earlier. There seemed to be some magic there that looked for these specs if they existed for a given rails version and run them. I somehow had the impression that there were not real tests being run via the appraisals for later versions without such version specific dirs. But yes, these are definitely unused right now. |
As long as we're talking about the Rails booter specs then than would depend on whether there's changes to the stuff we're adding/overriding. There's 2 groups (similar to the removed 2.3 group) left here:
The idea around
The integration spec mostly has those and there the removed group having |
Now having a second look, you're right. We should have some Rails skeleteon under src/spec/stub based on actual Rails version and use those as a (smoke) "integration" test. And I should also do more cleanup here, since I obviously missed a lot 😜. |
Managed to remove the Rails stub apps, and also looked into adding back (in a follow-up PR) some based on the Rails versions currently supported. I have a 7.2 passing tests locally and will setup another one likely for Rails 6.1 ( |
Sorry, missed your comment here. If you can point me to the branch where you're playing with adding back stubs/tests I can take a look and see if I can work it out, as there was some hacking around to do between Maven and rake to get things like that working on modern JRuby earlier. Maybe something similar. In my my main use case, I haven't been able to get Rails 7.1 working (let alone 7.2) and only managed to move to 7.0 recently after getting jruby-rack fixed via the PRs here, so I have less experience with what has changed on later Rails. I'm very surprised 7.2 actually works via your tests, so that's a good sign. For future it might be a good idea to have the build fail-fast if it can't find stub tests for a given Rails major/minor version to force us to add them and ensure completeness, if indeed they are important (which I think they probably are to validate something "real" even if we are not validating all manner of servlet containers, logging mechanisms and such). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
mostly just removing unused code/specs