Run action text system tests #33
Conversation
hahmed
changed the title
|
Actually... I still need to look into something. The change in this PR adds chrome: But the docker-compose file is already adding chrome: Dockerfile in Github: https://github.com/SeleniumHQ/docker-selenium/blob/trunk/NodeChrome/Dockerfile |
|
ActionView and ActionCable both use karma to run the tests, I removed this line Here is how I ran the integration tests for action view + action cable;
I'm hesitant in removing |
|
cc/ @rafaelfranca @yahonda this PR is ready and if this is ok, then rails/rails#47127 can also be merged. |
|
Can these tests not use the separate standalone-chrome instance? |
|
I could not really figure out how to get action text to use
Error: (I also tried
Maybe there is another way to set the path, but I'm not sure... Here is the docker image https://hub.docker.com/layers/selenium/standalone-chrome/latest/images/sha256-c070bad288d2175b5af1a05f4db4426602eb4d1e76f6692be0808b2353776682?context=explore Line 66 onwards. |
|
The binary won't be there on the machine running the test -- the point of the standalone image is that chrome runs in a separate docker image, and we communicate with it over the network (via |
|
Ah, thanks for that context, I previously attempted the url option, PR rails/rails#47030 but the tests were also failing |
|
I can't for the life of me figure out where we actually use |
|
@zzak atm it's not actually used, I think we need to pass in SELENIUM_URL as a remote option to fix these action text system tests |
|
I'm going to try again with that URL approach, because I feel like I'm missing something, I think the (Keep in mind this is me attempting to do this in a github action), I re-added that url and remote option here https://github.com/hahmed/rails/pull/1/files#diff-92bd81dd752a3bffacbc7de4ee005362d298dfc94905ae278b804f1c86097991 but my build is still failing https://github.com/hahmed/rails/actions/runs/6240789656/job/16941645072 If you manage to get it working, even better 😀 |
|
This is the same error I was getting when trying this in a repo at work, that was what brought me here 😭 |
|
@zzak just to clarify are you setting up cabybara tests for a rails app with that selenium docker image? (If so here is an example of it working https://github.com/hahmed/capybaraciiii/blob/85ce3c15d7882209eecbdf9ad4cb103bc9fc5570/.github/workflows/ci.yml) |
hahmed
force-pushed
the
ha/add-chrome
branch
from
4d3212c to
3d63dc6
Compare
hahmed
changed the title
hahmed
force-pushed
the
ha/add-chrome
branch
from
3d63dc6 to
a783b37
Compare
|
It looks like that chrome instance can be used after all 🤦🏽 From rails side, we don't have to do anything else but merge this PR. The only thing is I have it tested and working in a github ci action -- code: hahmed/rails@28d8385 and CI action that shows it's all green https://github.com/hahmed/rails/actions/runs/6286139237/job/17069264032 Will run the repro steps locally to test this out to see if everything works for buildkite (should be straightforward), however last time we got this error #26 (comment), need to figure out why. I don't have docker on this machine, will retest again 😅 -- going to close that Rails PR |
|
You should be able to test this in the CI job in this PR as well. It will create a full Buildkite build against main and 6.1 |
hahmed
force-pushed
the
ha/add-chrome
branch
from
a783b37 to
2896404
Compare
|
Even better @skipkayhil, although I think my builds are blocked/pending, might need approval or something but i'm not sure 🤔 |
If you are signed into Buildkite, you should see a button to review the build script and approve the job. I'm assuming you have the correct permissions though. |
|
Thanks @zzak @skipkayhil I OK'd and it's running, I'm assuming this change is all good to hit that ok button on. Got a failure now, so that's progress |
|
@hahmed That was my bad, sorry. I had left a broken config on the meta-ci, rebuilding now. 🙏 |
|
Also, I think the thing I realized is we're no longer using the chrome container (as of rails/rails@d9e79ce) where running karma on saucelabs instead. At least for Action View. I'm not sure if we want to go the same route for Action Text? But it feels like consistency might be a good argument. |
|
Here's the build failure:
|
|
Thanks @zzak i think Karma tests in rails for actioview/action cable are for javascript tests? Whereas these actiontext tests are system tests, so it needs rails, but i think you are right it will work also w/ saucelabs (not checked that though). There was a ton invested in buildkite, would love if we can just move everything over to github actions (I have this specific action working in a github action and did not need to change much). But that's prob a question for the team, maybe Matthew knows if it's ok to migrate to github 🤔 |
|
I think RF just fixed that failure about the I'm going to issue a rebuild. We should figure out how to get these tests passing on Buildkite. |
|
So it seems like chrome is just crashing, there is probably a flag we can add to fix that:
https://buildkite.com/rails/rails-ci/builds/155#018b0772-b4b1-4618-9ac4-3ad572693636 |
|
Tests are passing for me now, with this config locally: diff --git a/actiontext/test/application_system_test_case.rb b/actiontext/test/application_system_test_case.rb
index a24f473..dd1d2cb 100644
--- a/actiontext/test/application_system_test_case.rb
+++ b/actiontext/test/application_system_test_case.rb
@@ -3,7 +3,13 @@
require "test_helper"
class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
- driven_by :selenium, using: :headless_chrome
+ options = {
+ browser: ENV["SELENIUM_DRIVER_URL"].blank? ? :chrome : :remote,
+ url: ENV["SELENIUM_DRIVER_URL"].blank? ? nil : ENV["SELENIUM_DRIVER_URL"]
+ }
+ driven_by :selenium, using: :headless_chrome, options: options
end
Capybara.server = :puma, { Silent: true }
+Capybara.server_host = "0.0.0.0" # bind to all interfaces
+Capybara.app_host = "http://#{IPSocket.getaddress(Socket.gethostname)}" if ENV["SELENIUM_DRIVER_URL"].present? |
Blocks: rails/buildkite-config#33 Previous works: rails#47030, rails#47127 Co-authored-by: Haroon Ahmed <[email protected]>
Re-add system tests for action text (ended up reverting #26)
Rails PR that's needed to run the tests rails/rails#47127
To repro, as mentioned in this comment #26 (comment)