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

Add test:arel to test separately from adapters #37

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

skipkayhil
Copy link
Member

Previously, all of the Arel tests would be run with every database adapter. This is not necessarily a problem, but these tests end up running redundantly for each adapter/database combination even though they do not interact with adapters at all.

This commit follows up a commit in Rails [1] that added a new test:arel task for Active Record. This additional step creates a place for Arel to be tested a single time, so that a followup PR to Rails can filter out Arel tests when testing adapters.

1: rails/rails@f362f07

@@ -258,6 +258,7 @@ end
activestorage test default
actionmailbox test default
actiontext test default
activerecord test:arel default
Copy link
Member

Choose a reason for hiding this comment

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

Do we not care about parallelism of arel tests?

Copy link
Member

Choose a reason for hiding this comment

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

Nope, they're tiny. But as it'll only be present on some branches, I think this will need to be a bit more of a:

if REPO_ROOT.join("actionview/Rakefile").read.include?("task :ujs")
step_for("actionview", "test:ujs", service: "actionview")
end

@skipkayhil skipkayhil force-pushed the add-arel-test-step branch 2 times, most recently from 8d94ec8 to 0fc7b70 Compare March 3, 2023 16:49
@@ -299,6 +299,10 @@ end
if REPO_ROOT.join("actionview/Rakefile").read.include?("task :ujs")
step_for("actionview", "test:ujs", service: "actionview")
end
if REPO_ROOT.join("activerecord/Rakefile").read.include?("Rake::TestTask.new(:arel)")
Copy link
Member

Choose a reason for hiding this comment

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

It feels like this could break, like if we change that line in the Rakefile 🤔

Also, why can't we just do:

task :arel do |t|
  t.whatever
end

Copy link
Member Author

Choose a reason for hiding this comment

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

It feels like this could break, like if we change that line in the Rakefile 🤔

Yeah I see what you mean. I did this the same as task :ujs based on Matthew's suggestion but maybe his intention was more about making the test conditional and we could do a version check?

Also, why can't we just do:

in the task :ujs case its not ruby tests being run: the ujs task starts a rack server and then runs npm test. In the Arel case we want to run regular ruby tests so it makes sense to use Rake::TestTask.

@skipkayhil
Copy link
Member Author

skipkayhil commented Jun 26, 2023

Rebased to run against the new CI (this is super cool)

I'm now questioning whether this is a good idea. Since the task is so fast, it doesn't really impact the adapter tests other than it makes the output longer. On the other hand, the job is taking 54s in CI which doesn't seem worth the tradeoff at the moment

Edit: hmm, I wonder if we can skip checking out rails/rails on most of the jobs since we run in docker-compose anyway

Previously, all of the Arel tests would be run with every database
adapter. This is not necessarily a problem, but these tests end up
running redundantly for each adapter/database combination even though
they do not interact with adapters at all.

This commit follows up a [commit][1] in Rails that added a new test:arel
task for Active Record. This additional step creates a place for Arel to
be tested a single time, so that a followup PR to Rails can filter out
Arel tests when testing adapters.

Since the task is only present on the main branch, it cannot run for all
Rails versions and must be a special case.

[1] rails/rails@f362f07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants