Skip to content

Conversation

@theomelo
Copy link

@theomelo theomelo commented Mar 6, 2025

Closes: #120

  • Add and configure Appraisal.
  • Schedule rails_main tests to run independently from a PR push.
  • Configure GitHub Action to run against different Appraisal files:
    • Update .rspec to exclude specs in the ext directory.
    • Create a Rake task that will gather all gemfiles and run the command above with every gemfile.
    • Create a new action that will execute this new Rake task.
  • Add test coverage for lib/rage/ext/active_record/connection_pool.rb. Specifically:
    • ActiveRecord::Base.connection_pool.with_connection
    • ActiveRecord::Base.connection_pool.connection
    • ActiveRecord::Base.connection_pool.release_connection
    • ActiveRecord::Base.connection_pool.active_connection?

Copy link
Member

@rsamoilov rsamoilov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Theo, great job!

Have several comments right off the bat.

- Don't run `spec/ext` as part of the default test suite
- Separate running appraisals against the main rails branch. Instead,
  run the main branch as a scheduled action.
- Add a new rake task to run all appraisals (except against main Rails branch)
@theomelo
Copy link
Author

theomelo commented Mar 11, 2025

@rsamoilov I should've asked if you are against using force-push in your PRs before doing so here. I hope you don't mind. 🙇

I updated my PR with the changes you asked for; please let me know if I misunderstood anything. If the current changes are ok, I'll finish adding the tests for the connection_pool methods.

@theomelo theomelo requested a review from rsamoilov March 11, 2025 02:02
@rsamoilov
Copy link
Member

if you are against using force-push

Not at all.

Copy link
Member

@rsamoilov rsamoilov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks exactly as it should. Love it!

@rsamoilov
Copy link
Member

Oh, one more thing - let's remove the Appraisal file and the gemfiles directory from the bundle.

- Remove EOL Ruby version
- Fix steps identation
- Add caching for Appraisals gems
- Remove Appraisals' gemfiles folder from `gemspec`
@theomelo theomelo requested a review from rsamoilov March 15, 2025 00:47
@theomelo
Copy link
Author

I updated the workflow to use the cache, but I don't know if it will work as expected. I'll wait for you to approve and fix any errors that might happen.

Another project workflow question: do you prefer having one commit per Pull Request (squash them before merge)?

@rsamoilov
Copy link
Member

do you prefer having one commit per Pull Request (squash them before merge)

No, don't worry about the git stuff. Doesn't matter to me.

And sorry for the required approvals, I don't think it's possible to disable that.

@rsamoilov
Copy link
Member

rsamoilov commented Mar 16, 2025

Weird, it appears to be working, but it doesn't seem to change anything.
https://github.com/rage-rb/rage/actions/runs/13867738888/job/38848070523?pr=133

Screenshot 2025-03-16 at 15 28 27

@theomelo
Copy link
Author

And sorry for the required approvals, I don't think it's possible to disable that.

No worries! There's no reason to be sorry 🙏

Weird, it appears to be working, but it doesn't seem to change anything.
https://github.com/rage-rb/rage/actions/runs/13867738888/job/38848070523?pr=133

I'll investigate this today

@rsamoilov
Copy link
Member

Weird, it appears to be working, but it doesn't seem to change anything.

I wonder what will happen if you commit the lockfiles.

@theomelo
Copy link
Author

theomelo commented Apr 8, 2025

I wonder what will happen if you commit the lockfiles.

My apologies for the radio silence; I was recovering from an accident. I had them originally but removed them after this feedback. I'll add them back and we can see what will happen 🙏

@rsamoilov
Copy link
Member

My apologies for the radio silence; I was recovering from an accident.

I'm sorry to hear 😞 Get well soon!

I had them originally but removed them after this feedback

Right, I know I suggested removing them. But considering the cache doesn't work correctly, it is possible committing the lockfiles will fix the issue.

@rsamoilov
Copy link
Member

Hey @theomelo

I hope you're doing well.

You've made great progress here, and I'd love to merge the PR. I will continue working in this branch to get it merged, but let me know if you'd prefer to finish it yourself.

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.

🌶️ Active Record Tests

2 participants