Add test:arel to test separately from adapters #37
Conversation
pipeline-generate
Outdated
| @@ -258,6 +258,7 @@ end | |||
| activestorage test default | |||
| actionmailbox test default | |||
| actiontext test default | |||
| activerecord test:arel default | |||
There was a problem hiding this comment.
Do we not care about parallelism of arel tests?
There was a problem hiding this comment.
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:
buildkite-config/pipeline-generate
Lines 299 to 301 in b3bc915
skipkayhil
force-pushed
the
add-arel-test-step
branch
2 times, most recently
from
8d94ec8 to
0fc7b70
Compare
pipeline-generate
Outdated
| @@ -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)") | |||
There was a problem hiding this comment.
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
endThere was a problem hiding this comment.
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
force-pushed
the
add-arel-test-step
branch
from
0fc7b70 to
ed4c1e1
Compare
skipkayhil
force-pushed
the
add-arel-test-step
branch
from
ed4c1e1 to
d160fed
Compare
|
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 |
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
skipkayhil
force-pushed
the
add-arel-test-step
branch
from
d160fed to
7d95a97
Compare
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