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.

Sign up for GitHub

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

Fix system tests #569

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Fix system tests #569

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c8fd077
fix typo in system test
jlvallelonga Nov 30, 2024
37eed9b
updates test to stub app_url
jlvallelonga Nov 30, 2024
9a3d73d
fix system tests the right way?
jlvallelonga Nov 30, 2024
285a80b
try adding timeout retry & longer waits
krschacht Dec 4, 2024
c14ff71
Attempt another fix for image test
krschacht Dec 4, 2024
8e4dfbc
Revert
krschacht Dec 5, 2024
482aeb5
Merge branch 'main' into chore/fix-system-tests
krschacht Dec 7, 2024
5752d5d
Merge branch 'main' into chore/fix-system-tests
krschacht Dec 10, 2024
45bb2e1
fix selenium localhost calls with webmock
jlvallelonga Dec 12, 2024
2767bf3
fixes images_test
jlvallelonga Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,4 +87,5 @@ group :test do
gem "selenium-webdriver"
gem "minitest-stub_any_instance"
gem "rails-controller-testing"
gem "minitest-retry"
end
3 changes: 3 additions & 0 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,8 @@ GEM
mini_magick (4.13.2)
mini_mime (1.1.5)
minitest (5.25.1)
minitest-retry (0.2.3)
minitest (>= 5.0)
minitest-stub_any_instance (1.0.3)
mize (0.4.1)
protocol (~> 2.0)
Expand Down Expand Up @@ -493,6 +495,7 @@ DEPENDENCIES
gemini-ai (~> 4.2.0)
image_processing (~> 1.13.0)
importmap-rails
minitest-retry
minitest-stub_any_instance
name_of_person
omniauth (~> 2.1)
Expand Down
23 changes: 12 additions & 11 deletions test/system/conversations/messages/images_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class ConversationMessagesImagesTest < ApplicationSystemTestCase
end

test "images eventually render in messages WHEN NOT pre-processed, clicking opens modal" do
stimulate_image_variant_processing do
simulate_image_variant_processing do
visit_and_scroll_wait conversation_messages_path(@conversation)

image_msg = find_messages.third
Expand Down Expand Up @@ -86,11 +86,11 @@ class ConversationMessagesImagesTest < ApplicationSystemTestCase
end

test "ensure images display a spinner initially if they get a 404 and then eventually get replaced with the image" do
stimulate_image_variant_processing do
simulate_image_variant_processing do
visit_and_scroll_wait conversation_messages_path(@conversation)

image_msg = find_messages.third
image_btn = image_msg.find_role("image-preview")
image_btn = image_msg.find_role("image-preview")
loader = image_btn.find_role("image-loader")
img = image_btn.find("img", visible: :all)
modal_container = image_msg.find_role("image-modal")
Expand All @@ -103,15 +103,14 @@ class ConversationMessagesImagesTest < ApplicationSystemTestCase
refute img.visible?

image_btn.click

2.times do
sleep 0.1
sleep 0.5 if !modal_loader.visible?
sleep 0.1
image_btn.click if !modal_loader.visible?
end # TODO: sometimes modal has not popped up after clicking, why?? Try 2x times before failing the test.
Copy link
Contributor

Choose a reason for hiding this comment

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

@jlvallelonga I think we had 3 distinct test failures. You fixed one of them with the update to simulate_image_variant_processing. I tried to fix a sporadic Net::ReadTimeout with a timeout. However, there remains one more test failure on this line. I think it might be a legitimate bug.

When I turn off headless chrome and run just this specific test, I do not see the spinners in place of the images. I spent a little while investigating it and couldn't figure out why not. The thing I did not try was checking out an old commit right before the refactor to image attachments, and that would confirm for sure that we broke it recently.

Could you do a little more investigating of this one? What you should see when running bin/rails test test/system/conversations/messages/images_test.rb:88 in normal chrome mode is the spinners appearing for 8 seconds both inline in the conversation messages and also when you click on a spinner you see a spinner within the modal that opens. Then after 8 seconds (of faked delay) then the spinners are swapped out for the images.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok nice! yeah I'll dig into this last failure tomorrow


assert_true "modal image loader should be visible", wait: 0.6 do
assert_true "modal image loader should be visible", wait: 3 do
modal_loader.visible?
end
refute modal_img.visible?
Expand All @@ -134,7 +133,7 @@ class ConversationMessagesImagesTest < ApplicationSystemTestCase
end

test "ensure page scrolls back down to the bottom after an image pops in late" do
stimulate_image_variant_processing do
simulate_image_variant_processing do
visit_and_scroll_wait conversation_messages_path(@conversation)

image_msg = find_messages.third
Expand All @@ -156,7 +155,7 @@ class ConversationMessagesImagesTest < ApplicationSystemTestCase

test "images in previous messages remain after submitting a new message, they should not display a new spinner" do
image_msg = img = nil
stimulate_image_variant_processing do
simulate_image_variant_processing do
visit_and_scroll_wait conversation_messages_path(@conversation)

image_msg = find_messages.third
Expand Down Expand Up @@ -189,10 +188,12 @@ def preprocess_all_variants!
end
end

def stimulate_image_variant_processing(&block)
Document.stub_any_instance(:has_file_variant_processed?, false) do
ActiveStorage::PostgresqlController.stub_any_instance(:decode_verified_key, simulate_not_preprocessed) do
yield block
def simulate_image_variant_processing(&block)
stub_custom_config_value(:app_url, "not_nil") do
Document.stub_any_instance(:has_file_variant_processed?, false) do
ActiveStorage::PostgresqlController.stub_any_instance(:decode_verified_key, simulate_not_preprocessed) do
yield block
end
end
end
end
Expand Down
11 changes: 11 additions & 0 deletions test/test_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
require_relative "../config/environment"
require "rails/test_help"
require "minitest/autorun"
require "minitest/retry"
require "pry"

Dir[Rails.root.join("test/support/**/*.rb")].sort.each { |file| require file }
Expand All @@ -20,6 +21,12 @@ def exists?
end
end

Minitest::Retry.use!(
retry_count: 3,
verbose: true,
exceptions_to_retry: [Net::ReadTimeout]
)

class ActionDispatch::IntegrationTest
include Rails.application.routes.url_helpers

Expand Down Expand Up @@ -58,3 +65,7 @@ class TestCase
fixtures :all
end
end

class ActionDispatch::SystemTestCase
parallelize(workers: Etc.nprocessors/2)
end
Loading