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

Fix system tests #569

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

Fix system tests #569

wants to merge 10 commits into from

Conversation

jlvallelonga
Copy link
Contributor

@jlvallelonga jlvallelonga commented Nov 30, 2024

fixes mocks so that selenium can run

fixes images_test system test

@@ -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

@krschacht krschacht changed the title fix system tests Fix system tests Dec 5, 2024
@krschacht
Copy link
Contributor

@jlvallelonga Let me know if you any time to look into this failure. Or if things are busy on the home front, let me know and I can try to dig into this.

@jlvallelonga
Copy link
Contributor Author

Yeah busy week at home. I'll take a look at this now

@jlvallelonga
Copy link
Contributor Author

@krschacht I'm seeing errors like this when running system tests locally and on the github runners:

Error:
ConversationMessagesImagesTest#test_ensure_images_display_a_spinner_initially_if_they_get_a_404_and_then_eventually_get_replaced_with_the_image:
WebMock::NetConnectNotAllowedError: Real HTTP connections are disabled. Unregistered request: POST http://127.0.0.1:9515/session with body '{"capabilities":{"alwaysMatch":{"browserName":"chrome","goog:chromeOptions":{"args":["--headless"],"binary":"/usr/bin/google-chrome"}}}}' with headers {'Accept'=>'application/json', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Content-Length'=>'136', 'Content-Type'=>'application/json; charset=UTF-8', 'User-Agent'=>'selenium/4.25.0 (ruby linux)'}

Are you doing something to get around that?

@krschacht
Copy link
Contributor

@krschacht I'm seeing errors like this when running system tests locally and on the github runners:

Dang, I didn’t notice those. I get in this habit of once the system tests are broken I stop looking at them closely so I didn’t notice we had made them worse. I think it’s this change:
https://github.com/AllYourBot/hostedgpt/pull/567/files

Webmock was added along with some config for it. I don’t think we need webmock. I was able to mock up HTTP requests previously without using webmock so we should probably unwind that addition.

@jlvallelonga jlvallelonga marked this pull request as ready for review December 13, 2024 06:43
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.

2 participants