Commit Graph

231 Commits

Author SHA1 Message Date
Loïc Guitaut
ca4af53be8 DEV: Upgrade Rails to version 7.1 (#27539)
* DEV: Upgrade Rails to 7.1

* FIX: Remove references to `Rails.logger.chained`

`Rails.logger.chained` was provided by Logster before Rails 7.1
introduced their broadcast logger. Now all the loggers are added to
`Rails.logger.broadcasts`.

Some code in our initializers was still using `chained` instead of
`broadcasts`.

* DEV: Make parameters optional to all FakeLogger methods

* FIX: Set `override_level` on Logster loggers (#27519)

A followup to f595d599dd

* FIX: Don’t duplicate Rack response

---------

Co-authored-by: Jarek Radosz <jradosz@gmail.com>
2024-06-21 09:44:06 +02:00
Loïc Guitaut
982c005979 Revert "DEV: Upgrade Rails to version 7.1 (#27539)"
This reverts commit 2301dddcff.
2024-06-20 11:43:35 +02:00
Loïc Guitaut
2301dddcff
DEV: Upgrade Rails to version 7.1 (#27539)
* DEV: Upgrade Rails to 7.1

* FIX: Remove references to `Rails.logger.chained`

`Rails.logger.chained` was provided by Logster before Rails 7.1
introduced their broadcast logger. Now all the loggers are added to
`Rails.logger.broadcasts`.

Some code in our initializers was still using `chained` instead of
`broadcasts`.

* DEV: Make parameters optional to all FakeLogger methods

* FIX: Set `override_level` on Logster loggers (#27519)

A followup to f595d599dd

* FIX: Don’t duplicate Rack response

---------

Co-authored-by: Jarek Radosz <jradosz@gmail.com>
2024-06-20 10:33:01 +02:00
Jarek Radosz
5cb84f8dcf
DEV: Revert rails 7.1 upgrade (#27522)
* Revert "FIX: Set `override_level` on Logster loggers (#27519)"

This reverts commit c1b0488c54.

* Revert "DEV: Make parameters optional to all FakeLogger methods"

This reverts commit 3318dad7b4.

* Revert "FIX: Remove references to `Rails.logger.chained`"

This reverts commit f595d599dd.

* Revert "DEV: Upgrade Rails to 7.1"

This reverts commit 081b00391e.
2024-06-18 23:48:30 +02:00
Loïc Guitaut
081b00391e DEV: Upgrade Rails to 7.1 2024-06-18 15:58:05 +02:00
Alan Guo Xiang Tan
27efa2d8b7
DEV: Increment attempts for ce91767b90 (#27413)
If we don't increment attempts, we will retry forever.
2024-06-11 16:05:38 +08:00
Alan Guo Xiang Tan
ce91767b90
DEV: Try another workaround for getaddrinfo: Temporary failure in name resolution (#27410)
This commit tries another work around for the `Socket::ResolutionError: getaddrinfo: Temporary failure in name resolution`
error we are seeing on CI.

The problem with the previous workaround is that `Capybara.using_session` will attempt to resolve `localhost`
before yielding the block which means our retry code is not hit.

This problem may be related to https://bugs.ruby-lang.org/issues/20172
which hints at us potentially not being able to spin up threads on CI so
I'm adding a debugging statement when stuff fails.
2024-06-11 14:49:01 +08:00
Alan Guo Xiang Tan
3bff633ce0
DEV: Monkey patch Capybara.using_session to resolve localhost in CI (#27398)
This is a follow up to 9ff0805a1d. We
noticed that `localhost` can fail to resolve in other spots of the app
and not just in selenium-webdriver.

From the failing tests we have seen, the `getaddrinfo: Temporary failure in name resolution` error is only
seen from within the `Capybara.using_session` block. This commit aims to
ensure that `localhost` can be resolve after the new session is started.
2024-06-10 13:14:50 +08:00
Alan Guo Xiang Tan
9ff0805a1d
DEV: Monkey patch Selenium::WebDriver::Platform.localhost to retry (#27335)
On Github Actions, system tests which uses `Capybara#using_session` are
failing intermittently with the error "Socket::ResolutionError: getaddrinfo: Temporary failure in name resolution"
when `Selenium::WebDriver::Platform.localhost` tries to resolve
`localhost`.

Too much time has been spent trying to figure out why so we are giving
up here and just retrying the resolution of `localhost` on Github
Actions.
2024-06-05 07:54:15 +08:00
Alan Guo Xiang Tan
d68983e060
DEV: Use same Socket.getaddrinfo arguments as selenium-webdriver (#27301)
Follow up to c408b53689. We need better
debugging information
2024-06-03 13:11:40 +08:00
Alan Guo Xiang Tan
c408b53689
DEV: puts debugging information when CI encounters resolution errors (#27300)
We have been seeing flaky socket resolution errors on CI and need more
debugging information to figure out why
2024-06-03 10:26:02 +08:00
Alan Guo Xiang Tan
4d8eca91ef
Revert "DEV: Use 127.0.0.1 instead of localhost as Capybara's server host (#27215)" (#27218)
This reverts commit 998b50fdf4.

Ended up making system tests even more unstable
2024-05-28 11:32:22 +08:00
Alan Guo Xiang Tan
998b50fdf4
DEV: Use 127.0.0.1 instead of localhost as Capybara's server host (#27215)
We are seeing a weird resolution error on Github actions with the
following backtrace:

```
Failure/Error:
  visit File.join(
          GlobalSetting.relative_url_root || "",
          "/session/#{user.encoded_username}/become.json?redirect=false",
        )

Socket::ResolutionError:
  getaddrinfo: Temporary failure in name resolution

```

Switch to use `127.0.0.1` instead of forcing a name resolution.
2024-05-28 09:47:22 +08:00
Ted Johansson
69205cb1e5
DEV: Catch missing translations during test runs (#26258)
This configuration makes it so that a missing translation will raise an error during test execution. Better discover there than after deploy.
2024-05-24 22:15:53 +08:00
Martin Brennan
acc5b01e21
DEV: Add pry-stack_explorer again (#26763)
This was reverted in 26a387c9c6
because the other pry gem changes there broke prod -- it
should be safe to just add this dev/test dependency
2024-04-29 10:34:28 +10:00
David Taylor
26a387c9c6
Revert "DEV: Add pry-stack_explorer plugin gem (#26732)" (#26739)
This reverts commit 09f5af608f.

Moving all the `pry` gems to the development group broke `rails c` functionality in production
2024-04-24 13:03:35 +01:00
David Taylor
bca855f239
FIX: Improve handling of 'PublicExceptions' when bootstrap_error_pages enabled (#26700)
- Run the CSP-nonce-related middlewares on the generated response

- Fix the readonly mode checking to avoid empty strings being passed (the `check_readonly_mode` before_action will not execute in the case of these re-dispatched exceptions)

- Move the BlockRequestsMiddleware cookie-setting to the middleware, so that it is included even for unusual HTML responses like these exceptions
2024-04-24 09:40:13 +01:00
Martin Brennan
09f5af608f
DEV: Add pry-stack_explorer plugin gem (#26732)
This is only required in rails_helper, otherwise it is
not loaded. Allows for better debugging by allowing
navigation of the call stack from the point of `binding.pry`

c.f. https://github.com/pry/pry-stack_explorer
2024-04-24 14:35:21 +10:00
Martin Brennan
380e5ca6cb
DEV: Move more service code to core (#26613)
This is to enable :array type attributes for Contract
attributes in services, this is a followup to the move
of services from chat to core here:

cab178a405

Co-authored-by: Joffrey JAFFEUX <j.jaffeux@gmail.com>
2024-04-12 13:14:19 +02:00
Martin Brennan
8e08a3b31f
DEV: Use caller for plugin_file_from_fixtures (#26387)
Followup 0bbca318f2,
rather than making developers provide the plugin path
name (which may not always be the same depending on
dir names and git cloning etc) we can infer the plugin
dir from the caller in plugin_file_from_fixtures
2024-03-27 14:12:51 +11:00
Martin Brennan
0bbca318f2
DEV: Add plugin_file_from_fixtures helper (#26359)
This allows plugins to also easily read fixture
files for tests, rather than having to do stuff
like this:

```
File.open(File.join(__dir__, "../../../fixtures/100x100.jpg"))
```
2024-03-26 16:17:51 +10:00
Loïc Guitaut
ec9597db5a
DEV: Rely properly on selenium-manager for system specs (#26267) 2024-03-22 10:13:15 +08:00
Alan Guo Xiang Tan
4f24e3b3b2
DEV: Support running system tests using chromium and custom chromedriver (#26234)
Why this change?

Google does not yet publish binaries for chrome and chromedriver for
`linux/arm64`. In 484954ec4c, we attempted
to add support for running system tests on `linux/arm64` by switching to
Firefox but our system tests seem to make lots of assumptions about
running on chromium based browsers so there are some tests that don't work in Firefox.

This commit works around the lack of chrome and chromedriver binaries by
doing the following:

1. Adds a `DISCOURSE_SYSTEM_TEST_CHROMIUM` ENV variable which when set to
  `1` will allow us to run system tests using a chromium binary. Chromium
  binaries for `linux/arm64` are available and since Chrome is Chromium based, all of our 
  system tests "should pass" even when running against a Chromium binary. I don't expect 
  this to be perfect but I expect it to be better than running against Firefox. This change buys us time
  until Chrome finally ships binaries for `linux/arm64`.

2. Adds a `DISCOURSE_SYSTEM_TEST_CHROMEDRIVER_PATH` ENV variable to
   allow the chromedriver path to be configured. We need this because
   the [electron project](https://github.com/electron/electron/releases) actually
   releases chromewebdriver for `linux/arm64` so someone running
   `linux/arm64` can download the necessary chromedriver from the
   project instead of relying on selenium-manager.

This change is also important for us to support [discourse_test](https://github.com/discourse/discourse_docker/blob/main/image/discourse_test/Dockerfile) and [discourse_dev](https://github.com/discourse/discourse_docker/blob/main/image/discourse_dev/Dockerfile) images targeted at `linux/arm64`.
2024-03-19 14:47:14 +08:00
Martin Brennan
6bcbe56116
DEV: Use freeze_time_safe in more places (#25949)
Followup to 120a2f70a9,
uses new method to avoid time-based spec flakiness
2024-03-01 10:07:35 +10:00
Martin Brennan
42d203d773
DEV: Add ENV var to skip verbose gem backtrace in rspec failure (#25921)
In rspec request specs, we do a huge verbose backtrace
when there is an error. However, 99% of the time you don't
care about pages and pages of activesupport/rspec gem
LOC in the backtrace...so this commit introduces an
env var RSPEC_EXCLUDE_GEMS_IN_BACKTRACE to allow for
turning this off.
2024-02-28 12:31:12 +10:00
Alan Guo Xiang Tan
b64a58071d
DEV: Ensure that BlockRequestsMiddleware cookie is always set (#25826)
Why this change?

This reverts 725561cf4b as it did not
address the root cause of the problem even though it fixed the failing tests we were seeing 
when running `bundle exec rspec --tag ~type:multisite --order random:776 spec/system/admin_customize_form_templates_spec.rb spec/system/admin_sidebar_navigation_spec.rb spec/system/admin_site_setting_search_spec.rb spec/system/composer/dont_feed_the_trolls_popup_spec.rb spec/system/composer/review_media_unless_trust_level_spec.rb spec/system/create_account_spec.rb spec/system/editing_sidebar_tags_navigation_spec.rb spec/system/email_change_spec.rb spec/system/emojis/emoji_deny_list_spec.rb spec/system/group_activity_spec.rb spec/system/hashtag_autocomplete_spec.rb spec/system/network_disconnected_spec.rb spec/system/post_menu_spec.rb spec/system/post_small_action_spec.rb spec/system/tags_intersection_spec.rb spec/system/topic_list_focus_spec.rb spec/system/topic_page_spec.rb spec/system/user_page/user_profile_info_panel_spec.rb spec/system/viewing_group_members_spec.rb spec/system/viewing_navigation_menu_preferences_spec.rb`.

The root cause here is that `before_action`s added to a controller is
order dependent. As such, some requests were not setting the cookie
because the `before_action` callback was not even hit as a prior
`before_action` callbacks has raised an error such as the `check_xhr`
`before_action` callback.

To resolve the problem, we need to add the `prepend: true` option in
our monkey patch of `ApplicationController` to ensure that the
`before_action` callback which we have added is always run first.

This change also makes a couple of changes:

1. Improve the response body when a request is blocked by the `BlockRequestsMiddleware` middleware
   so that it makes debugging easier.

2. Only set the cookies for non-xhr HTML format requests. Setting it for
   other formats is kind of pointless.
2024-02-23 07:51:51 +08:00
Alan Guo Xiang Tan
6e9fbb5bab
DEV: Do not process requests initiated by browser in a different example (#25809)
Why this change?

We noticed that running `LOAD_PLUGINS=1 rspec --seed=38855 plugins/chat/spec/system/chat_new_message_spec.rb` locally
results in the system tests randomly failing. When we inspected the
request logs closely, we noticed that a `/presence/get` request from a
previous rspec example was being processed when a new rspec example is
already being run. We know it was from the previous rspec example
because inspecting the auth token showed the request using the auth
token of a user from the previous example. However, when a request using
an auth token from a previous example is used it ends up logging out the
same user on the server side because the user id in the cookie is the same
due to the use of `fab!`.

I did some research and there is apparently no way to wait until all
inflight requests by the browser has completed through capybara or
selenium. Therefore, we will add an identifier by attaching a cookie to all non-xhr requests so that
xhr requests which are triggered subsequently will contain the cookie in the request.

In the `BlockRequestsMiddleware` middleware, we will then reject any
requests when the value of the identifier in the cookie does not match the current rspec's example
location.

To see the problem locally, change `Auth::DefaultCurrentUserProvider.find_v1_auth_cookie` to the following:

```
  def self.find_v1_auth_cookie(env)
    return env[DECRYPTED_AUTH_COOKIE] if env.key?(DECRYPTED_AUTH_COOKIE)

    env[DECRYPTED_AUTH_COOKIE] = begin
      request = ActionDispatch::Request.new(env)
      cookie = request.cookies[TOKEN_COOKIE]

      # don't even initialize a cookie jar if we don't have a cookie at all
      if cookie&.valid_encoding? && cookie.present?
        puts "#{env["REQUEST_PATH"]} #{request.cookie_jar.encrypted[TOKEN_COOKIE]&.with_indifferent_access}"
        request.cookie_jar.encrypted[TOKEN_COOKIE]&.with_indifferent_access
      end
    end
  end
```

After which run the following command: `LOAD_PLUGINS=1 rspec --format documentation --seed=38855 plugins/chat/spec/system/chat_new_message_spec.rb`

It takes a few tries but the last spec should fail and you should see something like this:

```
assets/chunk.c16f6ba8b6824baa47ac.d41d8cd9.js {"token"=>"37d995a4b65395d3b343ec70fff915b4", "user_id"=>3382, "username"=>"bruce0", "trust_level"=>1, "issued_at"=>1708591735}
/assets/chunk.050148142e1d2dc992dd.d41d8cd9.js {"token"=>"37d995a4b65395d3b343ec70fff915b4", "user_id"=>3382, "username"=>"bruce0", "trust_level"=>1, "issued_at"=>1708591735}
/chat/api/channels/527/messages {"token"=>"37d995a4b65395d3b343ec70fff915b4", "user_id"=>3382, "username"=>"bruce0", "trust_level"=>1, "issued_at"=>1708591735}
/uploads/default/test_0/optimized/1X/_129430568242d1b7f853bb13ebea28b3f6af4e7_2_512x512.png {"token"=>"37d995a4b65395d3b343ec70fff915b4", "user_id"=>3382, "username"=>"bruce0", "trust_level"=>1, "issued_at"=>1708591735}
    redirects to existing chat channel
    redirects to chat channel if recipients param is missing (PENDING: Temporarily skipped with xit)
  with multiple users
/favicon.ico {"token"=>"9a75c114c4d3401509a23d240f0a46d4", "user_id"=>3382, "username"=>"bruce0", "trust_level"=>1, "issued_at"=>1708591736}
/chat/new-message {"token"=>"9a75c114c4d3401509a23d240f0a46d4", "user_id"=>3382, "username"=>"bruce0", "trust_level"=>1, "issued_at"=>1708591736}
/presence/get {"token"=>"37d995a4b65395d3b343ec70fff915b4", "user_id"=>3382, "username"=>"bruce0", "trust_level"=>1, "issued_at"=>1708591735}
 ```
 
 Note how the `/presence/get` request is using a token from the previous example. 

Co-authored-by: David Taylor <david@taylorhq.com>
2024-02-22 19:41:10 +08:00
Alan Guo Xiang Tan
d119ec617e
DEV: Disable BlockRequestsMiddleware before every test (#25712)
Why this change?

This is a follow up to c30aeafd9d. The
commit was calling `BlockRequestsMiddleware.allow_requests!` only before
`type: :system` tests but non system type tests could be running as well
and needs the `BlockRequestsMiddleware.allow_requests!` middleware to be
disabled too.
2024-02-16 07:01:36 +08:00
Alan Guo Xiang Tan
c30aeafd9d
DEV: Block all incoming requests before resetting Capybara session (#25692)
Why this change?

We have been debugging flaky system tests and noticed in https://github.com/discourse/discourse/actions/runs/7911902047/job/21596791343?pr=25690
that ActiveRecord connection checkout timeouts are encountered because
the Capybara server thread is processing requests even after
`Capybara.reset_session!` and ActiveRecord's `teardown_fixtures` have already been call.
The theory here is that an inflight request can still hit the Capybara
server even after `Capybara.reset_session!` has been called and end up
eating up an ActiveRecord connection for too long and also messing with
the database outside of a transaction.

What does this change do?

This change adds a `BlockRequestsMiddleware` middleware in the test
environment which is enabled to reject all incoming requests at the end
of each system test and before `Capybara.reset_session!` is called. At
the start of each RSpec test, the middleware is disabled again.
2024-02-15 16:36:12 +08:00
Alan Guo Xiang Tan
796af077c5
DEV: Debug AR connection pool queue on CI (#25687)
Why this change?

On CI, we have been seeing flaky system tests because ActiveRecord is
unable to checkout a connection. This patch is meant to help us debug
which thread is not returning the connection to the queue.
2024-02-15 14:00:30 +08:00
David Taylor
c37c00e9d8
DEV: Enable rspec full_cause_backtrace option (#25577)
This new rspec option shows the entire backtrace in the '--- Caused by: ---' section of RSpec failures.
2024-02-06 10:56:49 +00:00
Alan Guo Xiang Tan
06b7a6bd59
DEV: Accquire file lock before checking if selenium webdriver cache (#25534)
Why this change?

We are getting the following error on CI:

`Text file busy -
/github/home/.cache/selenium/chromedriver/linux64/121.0.6167.85/chromedriver`

This happens when two processes tries to download the chromedriver at
the same time. I'm not entirely sure why the previous implementatio is
not locking properly since we still saw the `Text file busy` error but
by accquring the lock before we even check for the existence of
`~/.cache/selenium`, we should be able to eliminate the chance of two
processes trying to download the chromedriver binary at the same time.
2024-02-02 11:04:37 +08:00
Alan Guo Xiang Tan
0bd3e03144
DEV: Fix system tests stuck on flock (#25495)
Why this change?

In workflow runs, we have seen processes being stuck on a flock lock and
I'm guessing because we are using `"w"` when opening the file which the
ruby documentation advises against as it states "don't use "w" because it truncates the file before lock."

Stuck workflow run: https://github.com/discourse/discourse/actions/runs/7690278010/job/20953851469
2024-01-31 11:11:40 +08:00
Loïc Guitaut
484954ec4c DEV: Add early support for aarch64 dev env
This patch allows running system specs on an aarch64 Linux system
(typically our `discourse_dev` docker image).
As Chrome isn’t available for the aarch64 architecture (yet), we have to
rely on Firefox instead. This has some drawbacks like not being able to
access the browser logs like we do with the Chrome webdriver.
2024-01-30 15:50:44 +01:00
Alan Guo Xiang Tan
0478b45fd3
DEV: Print out backtraces of all threads when spec times out on CI (#25356)
Why this change?

We have been seeing specs time out on GitHub CI but the problem is that
we are unable to debug those timeouts due to a lack of information. This
change seeks to print out the backtraces of all threads right before a
spec times out on CI.

What does this change do?

1. Starts a thread on CI which will wait for a spec to start running.
1. Once a spec starts running, the thread will sleep for
   `PER_SPEC_TIMEOUT_SECONDS -1` seconds.
1. After sleeping, the thread checks if the spec is still running and
   prints out the backtraces of all threads if it is. Otherwise, the
thread does nothing and runs the next loop.
1. At the end of each spec run, we ensure that the thread is in a
   waiting state for the next spec run to start.

Note that there is no need for us to teardown or cleanup the thread
since the process terminates after running all the tests.
2024-01-23 06:55:49 +08:00
Alan Guo Xiang Tan
54e6c1d823
DEV: Allow test-prof to be disabled completely with PREFABRICATION env (#25294)
Why this change?

We have been looking into a flaky system tests in one of our plugins
where the DB transaction flow can be messed up from time to time. Our
debugging effort is complicated by that fact that `test-prof` starts a
DB transaction in a `before(:all)` block which makes it hard to properly
log information. By allowing test-prof to be disabled completely, we
believe it will make it easier for us to isolate the problem we are
investigating.

What does this change do?

1. Avoid loading test-prof files if `PREFABRICATION` env has been set to
   `0`.

2. Set `PREFABRICATION=0` for plugin system tests in Github actions
2024-01-17 11:00:32 +08:00
Alan Guo Xiang Tan
e9f016726a
DEV: Minor formatting fix when reporting server exceptions (#25126)
What we have now:

```
~~~~~~~ SERVER EXCEPTIONS ~~~~~~~Error encountered while proccessing /tag/tag24/l/latest  ArgumentError: wrong number of arguments (given 1, expected 0)    /__w/discourse/discourse/lib/site_setting_extension.rb:521:in `block in setup_methods'
```

What we actually want:

```
~~~~~~~ SERVER EXCEPTIONS ~~~~~~~
Error encountered while proccessing /tag/tag24/l/latest  ArgumentError: wrong number of arguments (given 1, expected 0)    /__w/discourse/discourse/lib/site_setting_extension.rb:521:in `block in setup_methods'
```
2024-01-04 08:29:45 +08:00
Alan Guo Xiang Tan
655c106101
DEV: Capture and log AR debug logs on GitHub actions for flaky tests (#25048)
Why this change?

We have been running into flaky tests which seems to be related to
AR transaction problems. However, we are not able to reproduce this
locally and do not have sufficient information on our builds now to
debug the problem.

What does this change do?

Noe the following changes only applies when `ENV["GITHUB_ACTIONS"]` is
present.

This change introduces an RSpec around hook when `capture_log: true` has
been set for a test. The responsibility of the hook is to capture the
ActiveRecord debug logs and print them out.
2023-12-27 14:40:00 +08:00
Alan Guo Xiang Tan
bf3e121323
DEV: Set config.eager_load = true on CI (#25032)
Why this change?

When running system tests on our CI, we have been occasionally seeing
server errors like:

```
Error encountered while proccessing /stylesheets/desktop_e58cf7f686aab173f9b778797f241913c2833c39.css
  NoMethodError: undefined method `+' for nil:NilClass
    /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/path/pattern.rb:139:in `[]'
    /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:127:in `block (2 levels) in find_routes'
    /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:126:in `each'
    /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:126:in `each_with_index'
    /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:126:in `block in find_routes'
    /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:123:in `map!'
    /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:123:in `find_routes'
    /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:32:in `serve'
    /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/routing/route_set.rb:852:in `call'
```

While looking through various Rails issues related to the error above, I
came across https://github.com/rails/rails/pull/27647 which is a fix to
fully initialize routes before the first request is handled. However,
the routes are only fully initialize only if `config.eager_load` is set
to `true`. There is no reason why `config.eager_load` shouldn't be `true` in the
CI environment and this is what a new Rails 7.1 app is generated with.

What does this change do?

Enable `config.eager_load` when `env["CI"]` is present
2023-12-26 13:05:55 +08:00
Alan Guo Xiang Tan
a69b386556
DEV: Fix can't modify frozen string error when reporting server errors (#25033)
Follow up to 9d658591d6
2023-12-26 11:10:11 +08:00
Alan Guo Xiang Tan
9d658591d6
DEV: Fix nil exception when reporting rspec failures (#25011)
Follow-up to f5ca96528d

Why this change?

`RSpec.current_example.metadata[:extra_failure_lines]` can be `nil` and
calling `<<` on `nil` is not a good idea.

What does this change do?

Set `RSpec.current_example.metadata[:extra_failure_lines]` to `""` as
long as there are exceptions.
2023-12-22 13:41:45 +08:00
Alan Guo Xiang Tan
d3625f2288
DEV: Preload all models schema cache before running system tests (#25008)
Why this change?

When running system tests with all official plugins installed, we have
encountered instances where the system tests will hang. When dumping the
backtraces of the threads, we can see that the main thread running the
tests is stuck in a deadlock with the puma thread while serving a
request.

The deadlock happens when the main thread acquires the `ActiveSupport::Concurrency::LoadInterlockAwareMonitor`
lock first in `ActiveRecord::ConnectionAdapters::AbstractAdapter` before acquring another `Monitor` lock in
`ActiveRecord::ModelSchema`. In the Puma thread, it acquires the
`Monitor` lock in `ActiveRecord::ModelSchema` first before acquring the
`ActiveSupport::Concurrency::LoadInterlockAwareMonitor` lock.

What does this change do?

To workaround this problem, we will preload all model schema cache
before running system tests such that the `Monitor` lock in `ActiveRecord::ModelSchema`
will not be acquired.
2023-12-22 08:40:02 +08:00
Alan Guo Xiang Tan
20f950a116
DEV: Remove unnecessary method call in system tests teardown (#24998)
Why this change?

`Scheduler::Deferrable` runs in the async mode in the test environment
so there is no queue we need to flush.
2023-12-21 10:35:51 +08:00
Alan Guo Xiang Tan
f5ca96528d
DEV: Report all exceptions in RSpec example failure lines (#24981)
Why this change?

Previously, we were attaching any server exception to the RSpec
example's `Exception#cause` by doing `example.exception.cause =
RspecErrorTracker.last_exception`. However, this is problematic because
it relies on RSpec internal implementation details where RSpec will
print out the exception's cause. The other problem is that when RSpec
prints out the exception cause, it only includes a single line of
backtrace which isn't very helpful sometimes.

While this change of tracking the last exception works OK for request
specs, it doesn't not work for system specs where multiple requests can
be triggered in an example potentially leading to multiple exceptions.
Knowing all the exceptions which happened in the request is important
for us when it comes to debugging system test failures.

What does this change do?

`RspecErrorTracker` now tracks all exceptions that occurs during an
 RSpec example run. All the exceptions including the fullback trace of
each exception is printed out as part of the example's `extra_failure_lines` metadata.

Example:

```
Failures:

  1) Shortcuts | mark all read when chat is open when pressing shift+esc marks all channels read
     Failure/Error: expect(page).to have_content("all read messagasd")
       expected to find text "all read messagasd" in "Topics\nMy Posts\nReview\nAdmin\nMore\nCategories\nAmazing Category 0\nAmazing Category 1\nAmazing Category 2\nUncategorized\nAll categories\nConfigure defaults\nMessages\nInbox\nMy threads\nChannels\nKino Buffs 2\nMusic Lodge 0\nMusic Lodge 1\nPersonal chat\nMusic Lodge 1\nChat settings have been set to retain channel messages for 90 days.\nToday\nbruce6\n2:46 pm\nall read message 0\nbruce7\n2:46 pm\nall read message 1\nbruce8\n2:46 pm\nall read message 2\nbruce9\n2:46 pm\nall read message 3\nbruce10\n2:46 pm\nall read message 4\nbruce11\n2:46 pm\nall read message 5\nbruce12\n2:46 pm\nall read message 6\nbruce13\n2:46 pm\nall read message 7\nbruce14\n2:46 pm\nall read message 8\nbruce15\n2:46 pm\nall read message 9\nShowing all messages"

     [Screenshot Image]: /home/tgxworld/work/discourse/tmp/capybara/failures_r_spec_example_groups_shortcuts_mark_all_read_when_chat_is_open_when_pressing_shift_esc_marks_all_channels_read_236.png

     ~~~~~~~ SERVER EXCEPTIONS ~~~~~~~
     Error encountered while proccessing /stylesheets/desktop_theme_1_5dba82f48b7d6e4a9d54ffd915712811591356b7.css
       RuntimeError: boom
         /home/tgxworld/work/discourse/app/controllers/application_controller.rb:996:in `set_cross_origin_opener_policy_header'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:400:in `block in make_lambda'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:236:in `block in halting_and_conditional'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:599:in `block in invoke_after'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:599:in `each'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:599:in `invoke_after'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:133:in `block in run_callbacks'
         /home/tgxworld/work/discourse/app/controllers/application_controller.rb:423:in `block in with_resolved_locale'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/i18n-1.14.1/lib/i18n.rb:322:in `with_locale'
         /home/tgxworld/work/discourse/app/controllers/application_controller.rb:423:in `with_resolved_locale'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:127:in `block in run_callbacks'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:138:in `run_callbacks'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/abstract_controller/callbacks.rb:233:in `process_action'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_controller/metal/rescue.rb:23:in `process_action'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_controller/metal/instrumentation.rb:67:in `block in process_action'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/notifications.rb:206:in `block in instrument'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/notifications/instrumenter.rb:24:in `instrument'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/notifications.rb:206:in `instrument'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_controller/metal/instrumentation.rb:66:in `process_action'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_controller/metal/params_wrapper.rb:259:in `process_action'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activerecord-7.0.7/lib/active_record/railties/controller_runtime.rb:27:in `process_action'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/abstract_controller/base.rb:151:in `process'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionview-7.0.7/lib/action_view/rendering.rb:39:in `process'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_controller/metal.rb:188:in `dispatch'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_controller/metal.rb:251:in `dispatch'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/routing/route_set.rb:49:in `dispatch'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/routing/route_set.rb:32:in `serve'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:50:in `block in serve'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:32:in `each'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:32:in `serve'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/routing/route_set.rb:852:in `call'
         /home/tgxworld/work/discourse/lib/middleware/omniauth_bypass_middleware.rb:64:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/tempfile_reaper.rb:15:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/conditional_get.rb:27:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/head.rb:12:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/http/permissions_policy.rb:38:in `call'
         /home/tgxworld/work/discourse/lib/content_security_policy/middleware.rb:12:in `call'
         /home/tgxworld/work/discourse/lib/middleware/anonymous_cache.rb:351:in `call'
         /home/tgxworld/work/discourse/lib/middleware/gtm_script_nonce_injector.rb:10:in `call'
         /home/tgxworld/work/discourse/spec/rails_helper.rb:47:in `call'
         /home/tgxworld/work/discourse/config/initializers/008-rack-cors.rb:14:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/session/abstract/id.rb:266:in `context'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/session/abstract/id.rb:260:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/cookies.rb:704:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/callbacks.rb:27:in `block in call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7/lib/active_support/callbacks.rb:99:in `run_callbacks'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/callbacks.rb:26:in `call'
         /home/tgxworld/work/discourse/plugins/discourse-geoblocking/lib/geoblocking_middleware.rb:24:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/debug_exceptions.rb:28:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/show_exceptions.rb:29:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/railties-7.0.7/lib/rails/rack/logger.rb:40:in `call_app'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/railties-7.0.7/lib/rails/rack/logger.rb:27:in `call'
         /home/tgxworld/work/discourse/config/initializers/100-quiet_logger.rb:20:in `call'
         /home/tgxworld/work/discourse/config/initializers/100-silence_logger.rb:29:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/remote_ip.rb:93:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/request_id.rb:26:in `call'
         /home/tgxworld/work/discourse/lib/middleware/enforce_hostname.rb:24:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/method_override.rb:24:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/executor.rb:14:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/static.rb:23:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/sendfile.rb:110:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/middleware/host_authorization.rb:131:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/message_bus-4.3.8/lib/message_bus/rack/middleware.rb:60:in `call'
         /home/tgxworld/work/discourse/lib/middleware/request_tracker.rb:233:in `call'
         /home/tgxworld/work/discourse/config/initializers/200-first_middlewares.rb:27:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/railties-7.0.7/lib/rails/engine.rb:530:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/urlmap.rb:74:in `block in call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/urlmap.rb:58:in `each'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/urlmap.rb:58:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/rack-2.2.8/lib/rack/builder.rb:244:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/capybara-3.39.2/lib/capybara/server/animation_disabler.rb:25:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/capybara-3.39.2/lib/capybara/server/middleware.rb:60:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/puma-6.4.0/lib/puma/configuration.rb:272:in `call'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/puma-6.4.0/lib/puma/request.rb💯in `block in handle_request'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/puma-6.4.0/lib/puma/thread_pool.rb:378:in `with_force_shutdown'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/puma-6.4.0/lib/puma/request.rb:99:in `handle_request'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/puma-6.4.0/lib/puma/server.rb:443:in `process_client'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/puma-6.4.0/lib/puma/server.rb:241:in `block in run'
         /home/tgxworld/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/puma-6.4.0/lib/puma/thread_pool.rb:155:in `block in spawn_thread'
     ~~~~~~~ END SERVER EXCEPTIONS ~~~~~~~

     ~~~~~~~ JS LOGS ~~~~~~~
     http://localhost:31337/stylesheets/desktop_theme_1_5dba82f48b7d6e4a9d54ffd915712811591356b7.css?__ws=localhost - Failed to load resource: the server responded with a status of 500 (Internal Server Error)
     ~~~~~ END JS LOGS ~~~~~
```
2023-12-20 15:17:12 +08:00
Alan Guo Xiang Tan
4ee381fef3
DEV: Remove extra calls to reset_sessions! and use_default_driver (#24977)
Why this change?

Capybara is already calling those two methods: 52eaecea6d/lib/capybara/rspec.rb (L20-L21)
2023-12-20 12:27:49 +08:00
Alan Guo Xiang Tan
ac21d8af50
DEV: Set Capybara.default_max_wait_time to 4 as default (#24934)
Why this change?

By default, `Capybara.default_max_wait_time` is set to `2`. However,
this is not a high enough default for Discourse as certain requests like
creating a post can take upwards of 2 seconds even on a high end desktop
CPU like the Ryzen 5950x. Therefore, we have decided to double the default max wait time.
2023-12-18 11:51:59 +08:00
David Taylor
6731eec42a
DEV: Summarize JS deprecations at end of system spec run (#24824) 2023-12-13 16:04:25 +00:00
Jarek Radosz
86b9e784dc
DEV: Re-enable browser read timeout in specs (#24844)
Turns out there's an easier (and properly working) way to do this!

See: 52eaecea6d/lib/capybara/selenium/driver.rb (L68C42-L68C42)
2023-12-12 12:29:23 +01:00
Alan Guo Xiang Tan
775dce1f13
DEV: Partially revert 5b91dc1844 (#24838)
Why this change?

The code changes introduced in 5b91dc1844
resulted in errors being raised when `session.quit` is called when using
multiple sessions. From my debugging, this seems to be attributed to the
fact that the change introduced resulted in multiple sessions sharing
the same instance of `Selenium::WebDriver::Remote::Http::Default`. While
sharing the same instance in theory should be fine, but the problem is
that `Selenium::WebDriver::Driver` will mutate the `server_url` of the
client in `Selenium::WebDriver::Remote::Bridge`. This is problematic
because each session created by capbyara relies on a different server
URL and this mutation causes all sorts of weird errors to occur.

To reproduce the problem, run `LOAD_PLUGINS=1 rspec plugins/chat/spec/system/send_message_spec.rb:76`
locally while excluding the changes in this commit.
2023-12-12 12:39:23 +08:00
Jarek Radosz
601510f730
DEV: Increase per-spec timeout to 45s (#24798) 2023-12-08 21:14:41 +01:00