Commit Graph

178 Commits

Author SHA1 Message Date
Daniel Waterworth 6e161d3e75
DEV: Allow fab! without block (#24314)
The most common thing that we do with fab! is:

    fab!(:thing) { Fabricate(:thing) }

This commit adds a shorthand for this which is just simply:

    fab!(:thing)

i.e. If you omit the block, then, by default, you'll get a `Fabricate`d object using the fabricator of the same name.
2023-11-09 16:47:59 -06:00
Alan Guo Xiang Tan 9b5ab7a764
DEV: Allow Capybara's server port to be configurable (#23606)
Why this change?

This allows scripts to configure the server port and avoid hardcoding
the default port that is used in Discourse core.
2023-09-15 11:46:03 +08:00
Daniel Waterworth 290306a932
SECURITY: Reduce maximum size of SVG sprite cache to prevent DoS
Co-authored-by: Penar Musaraj <pmusaraj@gmail.com>
2023-09-12 15:31:28 -03:00
Alan Guo Xiang Tan 8ce5b82aa5
DEV: Allow `CAPYBARA_REMOTE_DRIVER_URL` through webmock (#23476)
Why this change?

When using a remote capybara driver configured through the
`CAPYBARA_REMOTE_DRIVER_URL` env, webmock is thinking that is an
external request and blocking it. As such, we need to set the URL to the
allowlist for webmock.
2023-09-08 11:17:08 +08:00
Alan Guo Xiang Tan 6beed147cd
DEV: Add `CAPYBARA_SERVER_HOST` (#23475)
Why this change?

When running in a Docker container, we want to bind the Rails server
started by Capybara to 0.0.0.0 instead of localhost. This is done via
the `server_host` config for Capybara which can now be configured via
the `CAPYBARA_SERVER_HOST` env.
2023-09-08 11:16:48 +08:00
Alan Guo Xiang Tan 71595647a5
DEV: Add env to configure Capybara to run against a remote chrome (#23453)
What motivated this change?

We are currently working on allowing system tests to be run within a
Docker container. While system tests are usually ran in chrome headless
mode, it is useful to also be able to run the system tests with chrome
in the non-headless mode. However, running a GUI application from within
a docker container is not usually recommended and from our research
quite difficult. As such, we want to allow running system tests against
a remote browser.

For example, one can run a `chromedriver` server on localhost and then
configure Capybara to connect to the `chromedriver` from within the
container.

What does this change do?

This change adds support for a `CAPYBARA_REMOTE_DRIVER_URL` env variable
which will switch Capybara to use the remote driver instead of the
`chrome` driver. Currently, we expect the remote driver to be a
`chromedriver` server.
2023-09-07 14:07:17 +08:00
Martin Brennan cf42466dea
DEV: Add S3 upload system specs using minio (#22975)
This commit adds some system specs to test uploads with
direct to S3 single and multipart uploads via uppy. This
is done with minio as a local S3 replacement. We are doing
this to catch regressions when uppy dependencies need to
be upgraded or we change uppy upload code, since before
this there was no way to know outside manual testing whether
these changes would cause regressions.

Minio's server lifecycle and the installed binaries are managed
by the https://github.com/discourse/minio_runner gem, though the
binaries are already installed on the discourse_test image we run
GitHub CI from.

These tests will only run in CI unless you specifically use the
CI=1 or RUN_S3_SYSTEM_SPECS=1 env vars.

For a history of experimentation here see https://github.com/discourse/discourse/pull/22381

Related PRs:

* https://github.com/discourse/minio_runner/pull/1
* https://github.com/discourse/minio_runner/pull/2
* https://github.com/discourse/minio_runner/pull/3
2023-08-23 11:18:33 +10:00
Penar Musaraj 10c6b2a0c2
WIP: Rename Webauthn to DiscourseWebauthn (#23077) 2023-08-18 08:39:10 -04:00
Alan Guo Xiang Tan 8b3eca056b
DEV: Fix chromedriver binary errors when running system tests in parallel (#23122)
What is the problem here?

The `selenium-webdriver` gem is responsible for downloading the
right version of the `chromedriver` binary and it downloads it into the
`~/.cache/selenium` folder. THe problem here is that when a user runs `bin/turbo_rspec spec/system`
for the first time, all of the processes will try to download the
`chromedriver` binary to the same path at the same time and will lead
to concurrency errors.

What is the fix here?

Before running any RSpec suite, we first check if the `.cache/selenium`
folder is present. If it is not present, we use a file system lock to
download the `chromedriver` binary such that other processes that runs
after will not need to install the `chromedriver` binary.

The long term fix here is to get `selenium-manager` to download the `chromedriver` binary to a unique path for each
process but the `--cache-path` option for `selenium-manager` is currently not supported in `selenium-webdriver`.
2023-08-17 12:53:40 +08:00
Mark VanLandingham fbf7b106cc
DEV: Bump selenium-webdriver version to fix system spec running (#23117)
We can no long user Webdriver - SeleniumHQ/selenium#11066. Bumping selenium-webdriver did the trick, as well as manually setting the user_agent for mobile system specs. Unsure what changed to make this necessary, but it is necessary to get the app to boot in mobile view.
2023-08-16 15:07:03 -05:00
Alan Guo Xiang Tan 7954d34448
DEV: Clean up more state in between system tests (#23009)
Why this change?

By default in the test environment, MessageBus used the memory backend
which means all messages are stored in an in-memory data structure. However,
the in-memory data structure is not cleared after each system test so we
have the potential to be leaking stuff between system tests.

Similarly for the defer queue which process work in another thread, we
want to ensure that the defer queue processes everything it has to do
before the transaction is rolled back.
2023-08-10 07:32:27 +08:00
David Taylor ac85520813
DEV: Only reveal capybara finder timeouts if the spec otherwise passes (#23026)
Followup to edb276b9a9
2023-08-09 11:56:09 +01:00
David Taylor edb276b9a9
DEV: Raise exception when capybara finder times out (#22686)
If a selenium finder takes the full wait duration to resolve, that means it has been written inefficiently. Most likely a matcher has been negated incorrectly.

This commit introduces a patch which will raise an error in this situation so that we can catch the issues while developing specs.

This commit also fixes chat's visit_thread helper. It was spinning on `has_css?(".chat-skeleton")` for the full selenium wait duration, and then returns false. That's because the thread is often already fully loaded before `has_css?` is even called. It's now updated to only look for the final expected state.
2023-08-08 10:16:09 +01:00
Alan Guo Xiang Tan bfc3132bb2
SECURITY: Impose a upper bound on limit params in various controllers
What is the problem here?

In multiple controllers, we are accepting a `limit` params but do not
impose any upper bound on the values being accepted. Without an upper
bound, we may be allowing arbituary users from generating DB queries
which may end up exhausing the resources on the server.

What is the fix here?

A new `fetch_limit_from_params` helper method is introduced in
`ApplicationController` that can be used by controller actions to safely
get the limit from the params as a default limit and maximum limit has
to be set. When an invalid limit params is encountered, the server will
respond with the 400 response code.
2023-07-28 12:53:46 +01:00
OsamaSayegh 0976c8fad6
SECURITY: Don't reuse CSP nonce between anonymous requests 2023-07-28 12:53:44 +01:00
Krzysztof Kotlarek 77b4e42f61
FIX: specify chrome version (#22681) 2023-07-19 15:06:56 +10:00
Martin Brennan 2cc353104e
DEV: Add more chrome options for system specs (#22659)
* CHROME_LOAD_EXTENSIONS_MANIFEST - An env var with a path to a file
  that contains one path per line. These are paths to extensions installed
  in chrome that the user wants to load while running system specs.
  Useful to run things like Ember Inspector.
* CHROME_DISABLE_FORCE_DEVICE_SCALE_FACTOR - On some systems the
  --force-device-scale-factor=1 argument makes the UI for chrome
  super small, add a way to disable this.
2023-07-19 10:09:34 +10:00
Martin Brennan 6459922993
DEV: Move Bookmark modal/component to use d-modal (#22532)
c.f. https://meta.discourse.org/t/converting-modals-from-legacy-controllers-to-new-dmodal-component-api/268057

This also converts the Bookmark component to a Glimmer
component.
2023-07-17 10:14:17 +10:00
Andrei Prigorshnev fbe0e4c78c
DEV: make sure we don't load all data into memory when exporting chat messages (#22276)
This commit makes sure we don't load all data into memory when doing CSV exports. 
The most important change here made to the recently introduced export of chat 
messages (3ea31f4). We were loading all data into memory in the first version, with 
this commit it's not the case anymore.

Speaking of old exports. Some of them already use find_each, and it worked as 
expected, without loading all data into memory. And it will proceed working as 
expected after this commit.

In general, I made sure this change didn't break other CSV exports, first manually, and 
then by writing system specs for them. Sadly, I haven't managed yet to make those 
specs stable, they work fine locally, but flaky in GitHub actions, so I've disabled them 
for now.

I'll be making more changes to the CSV exports code soon, those system specs will be 
very helpful. I'll be running them locally, and I hope I'll manage to make them stable 
while doing that work.
2023-07-12 18:52:18 +04:00
Daniel Waterworth b7404373cf
FIX: Always clear caches after committing the current transaction (#22550)
Instead of having to remember every time, just always wait until the
current transaction (if it exists) has committed before clearing any
DistributedCache.

The only exception to this is caches that aren't caching things from
postgres.

This means we have to do the test setup after setting the test
transaction, because doing the test setup involves clearing caches.

Reapplying this - it now doesn't use after_commit if skip_db is set
2023-07-12 09:49:28 -05:00
Daniel Waterworth cb794275a7
Revert "FIX: Always clear caches after committing the current transaction" (#22493)
This reverts commit 8310c7842c.

This was breaking precompilation.
2023-07-07 18:05:38 -05:00
Daniel Waterworth 9dd01ca2ef
FIX: Always clear caches after committing the current transaction (#22491)
Instead of having to remember every time, just always wait until the
current transaction (if it exists) has committed before clearing any
DistributedCache.

The only exception to this is caches that aren't caching things from
postgres.

This means we have to do the test setup after setting the test
transaction, because doing the test setup involves clearing caches.
2023-07-07 14:24:56 -05:00
Daniel Waterworth 5e2edeb2f7
FIX: Set test transaction to before_all transaction (#22445)
So that after_commit hooks work correctly for pre-fabricated objects.
2023-07-06 12:23:28 -05:00
Alan Guo Xiang Tan e1c3c7bddf
DEV: Remove ActiveRecord connection pool busy check in tests (#22460)
This was added way back in bdf3da8f80 but
has never been useful in any meaningful way for us. Therefore, we're
dropping this check.
2023-07-06 12:02:28 +08:00
Martin Brennan ccdc0822a8
DEV: Fix flaky thread navigation spec (#22323)
Introduced in cec68b3e2c,
this is flaky because if you click the back button before
the route is fully transitioned to the loaded thread,
we end up going to the history _before_ the thread list,
which ends up being the channel.

We need to make sure that everything is loaded for the
thread first, meaning the skeleton is not there.

Also exclude some noise from the capybara logs (image load failures)
2023-06-28 17:20:05 +10:00
Jarek Radosz b7568ea4a5
FIX: TOC anchors in a subfolder setup (#21985)
Clicking on TOC heading anchors in a subfolder setup was breaking the current URL for users.

Other than the fix this change introduces the ability to test the subfolder setup in system specs.
2023-06-12 13:59:54 +02:00
Alan Guo Xiang Tan 41f8bff2c3
DEV: Remove superfluous `js: true` metadata (#21960)
Why this change?

It is very unlikely that we need to ever JS for system tests considering
that we rely on a JS framework on the frontend.
2023-06-07 09:26:58 +08:00
Osama Sayegh 6cf863080a
DEV: Eliminate flakiness in specs that depend on plugins from fixtures (#21912) 2023-06-05 08:06:00 +08:00
Alan Guo Xiang Tan 0d0ca075a6
Revert "DEV: Improve performance of system tests by disabling GPU in chrome (#21882)" (#21888)
This reverts commit 128d67ba56.

Doesn't seem to have any impact on CI
2023-06-01 22:05:04 +08:00
Alan Guo Xiang Tan 128d67ba56
DEV: Improve performance of system tests by disabling GPU in chrome (#21882)
It seems like the overhead of GPU acceleration is not worth it and is
slowing down our system tests. Locally the following command completes
in `2 minutes 8.6 seconds` with GPU disabled as compared to `2 minutes 45.4 seconds` with GPU enabled.
2023-06-01 16:59:20 +08:00
Alan Guo Xiang Tan 4dd053a69c
DEV: Call `MessageBus.stop()` after each system test (#21848)
## What is the problem?

MessageBus by default uses long polling which keeps a connection
open for 25 seconds by default. The problem here is that Capybara does not know about these
connections being kept opened by MessageBus and hence does not know how
to stop these connections at the end of each test. As a result, the long polling MessageBus connections are kept opened by the browser and we hit chrome's limit of 6 concurrent requests per host, new request made in the browser is marked as "pending" until a request is freed up. Since we keep a MessageBus long polling connection opened for 25 seconds, our finders in Capybara end up hitting Capybara's wait time out causing the tests to fail.  

## What is the fix?

Since we can't rely on Capybara to close all the existing Capybara
connections, we manually execute a script to stop all MessageBus
connections after each system test.

```
for i in {1..10}; do
  echo "Running iteration $i"
  PARALLEL_TEST_PROCESSORS=8 CAPYBARA_DEFAULT_MAX_WAIT_TIME=10 bin/turbo_rspec --seed=34908 --profile --verbose --format documentation spec/system
  if [ $? -ne 0 ]; then
    echo "Error encountered on iteration $i"
    exit 1
  fi
done
echo "All 10 iterations completed successfully"
```

Without the fix, the script fails consistently in the first few iterations. Running in non-headless mode with the "network" tab opened will reveal the requests that are marked as pending.
2023-05-31 21:00:35 +08:00
Jarek Radosz 3569a48b2d
DEV: Rescue the timeout error for a better spec cleanup (#21826)
Rescuing them still makes timing-out tests fail but doesn't break `after` spec cleanup (which could trigger more errors) Using custom error class to avoid any other possible timeout-catching code.

Also:
* remove an unnecessary `.select { |x| x.size > 0 }`
* fix a typo in a test title
2023-05-30 19:14:54 +02:00
Alan Guo Xiang Tan b00edf3ea0 DEV: Add `--profile=[COUNT]` option for `turbo_rspec`
Why is this change required?

By default, `RSpec` comes with a `--profile=[COUNT]` option as well but
enabling that option means that the entire test suite needs to be
executed. This does not work so well for `turbo_rspec` which splits our
test files into various "buckets" for the tests to be executed in
multiple processes. Therefore, this commit adds a similar
`--profile=[COUNT]` option to `turbo_rspec` but will only profile the
tests being executed. Examples:

`LOAD_PLUGINS=1 bin/turbo_rspec --profile plugins/*/spec/system`

or

`LOAD_PLUGINS=1 bin/turbo_rspec --profile=20 plugins/*/spec/system`
2023-05-30 13:46:14 +09:00
Alan Guo Xiang Tan b580f04d34
DEV: Disable animation in system tests (#21821)
What is this change?

This change is an attempt to avoid flakiness in our tests due to
animations being enabled in our tests. An example of flakiness caused by
animations is when the `find(selector).click` pattern is used. When
`find(selector)` returns the node, its position may have changed if the
element is still moving. However, the `click` method will end up
clicking on the old position.

Either way, there is no need for us to make system tests even more
complicated by enabling animations.
2023-05-30 12:45:24 +08:00
Joffrey JAFFEUX 885ab7c7bb
DEV: allows to open chrome tools by default (#21778)
Usage:

```
CHROME_DEV_TOOLS=bottom  bundle exec rspec /path/to/system/spec
```

This commit also regroups common chrome options under `apply_base_chrome_options`, and removes the size of the mobile window which was incorrect. browser_log param is also passed to mobile chrome options.
2023-05-28 14:16:38 +02:00
Alan Guo Xiang Tan dab37c1a5a
DEV: Run tests with new chrome headless (#21684)
New headless shares the same implementation as the chrome browser
instead of being a separate implementation of its own.

See https://developer.chrome.com/articles/new-headless/ for more
details

Co-authored-by: Rafael dos Santos Silva <xfalcox@gmail.com>
2023-05-26 11:02:56 +02:00
Joffrey JAFFEUX 38d358fb9a
FIX: flakey spec in sidebar with new font size (#21728)
This commit also includes two changes to the rails helper which make tests more consistent on different devices. With this change the failure was reproducible locally and not only on CI:

```
options.add_argument("--force-device-scale-factor=1")
```

The fix itself is quite simple and attempts to find safe click coordinates, the previous solution could fail depending on the size of the sidebar.
2023-05-24 18:59:12 +02:00
Jarek Radosz 928d608ce7
DEV: Add a per-spec timeout (#21648) 2023-05-19 12:08:48 +02:00
Jarek Radosz eec10efc3d
DEV: Enable color CI output and tweak formatting (#21527)
* Color for turbo_rspec in CI (`progress` and `documentation` formats)
* Show "DONE" only when `documentation` formatter is used
* Fix formatting
* Collapse RSpec commands
* Add line wrapping to the `progress` formatter (to mitigate GH Actions issue)
2023-05-12 18:22:15 +02:00
Jarek Radosz fc17045876
DEV: Clean up workflow files (#21526) 2023-05-12 14:00:04 +02:00
Alan Guo Xiang Tan fe10c61dfa
DEV: Reset capybara sessions and default driver after each test (#21402)
I don't think we're leaking state at the moment but the docs are
recommending that this two methods are called after each run.
2023-05-05 11:37:04 +08:00
Joffrey JAFFEUX bf886662df
UX: improves composer and thread panel (#21210)
This pull request is a full overhaul of the chat-composer and contains various improvements to the thread panel. They have been grouped in the same PR as lots of improvements/fixes to the thread panel needed an improved composer. This is meant as a first step.

### New features included in this PR

- A resizable side panel
- A clear dropzone area for uploads
- A simplified design for image uploads, this is only a first step towards more redesign of this area in the future

### Notable fixes in this PR

- Correct placeholder in thread panel
- Allows to edit the last message of a thread with arrow up
- Correctly focus composer when replying to a message
- The reply indicator is added instantly in the channel when starting a thread
- Prevents a large variety of bug where the composer could bug and prevent sending message or would clear your input while it has content

### Technical notes

To achieve this PR, three important changes have been made:

- `<ChatComposer>` has been fully rewritten and is now a glimmer component
- The chat composer now takes a `ChatMessage` as input which can directly be used in other operations, it simplifies a lot of logic as we are always working a with a `ChatMessage`
- `TextareaInteractor` has been created to wrap the existing `TextareaTextManipulation` mixin, it will make future migrations easier and allow us to have a less polluted `<ChatComposer>`

Note ".chat-live-pane" has been renamed ".chat-channel"

Design for upload dropzone is from @chapoi
2023-04-25 10:23:03 +02:00
Martin Brennan 360d0dde65
DEV: Change Bookmarkable registration to DiscoursePluginRegistry (#20556)
Similar spirit to e195e6f614,
this moves the Bookmarkable registration to DiscoursePluginRegistry
so plugins which are not enabled do not register additional
bookmarkable classes.
2023-03-08 10:39:12 +10:00
Alan Guo Xiang Tan 925a7bd48b
DEV: Port sidebar mobile view acceptance tests to system tests (#20421)
The acceptance tests are flaky so I've decided to port them to system
test which makes them easier to work with and reason about.
2023-02-23 15:01:39 +08:00
Martin Brennan 07ab20131a
FEATURE: Chat side panel with threads initial skeleton (#20209)
This commit introduces the skeleton of the chat thread UI. The
structure of the components looks like this. Its done this way
so the side panel can be used for other things as well if we wish,
not just for threads:

```
.main-chat-outlet
   <ChatLivePane />
   <ChatSidePanel>
     <-- rendered with {{outlet}} -->
     <ChatThread />
   </ChatSidePanel>
```

Later on the `ChatThreadList` will be rendered here as well.
Now, when you go to a channel you can open a thread by clicking
on either the Open Thread message action button or by clicking on
the reply indicator. This will take you to a route like `chat/c/:slug/:channelId/t/:threadId`.
This works on mobile as well.

This commit includes basic serializers and routes for threads,
as well as a new `ChatThreadsManager` service in JS that caches
threads for a channel the same way the channel threads manager does.

The chat messages inside the thread are intentionally left out
until a later PR.

**NOTE: These changes are gated behind the site setting enable_experimental_chat_threaded_discussions
and the threading_enabled boolean on a ChatChannel**
2023-02-14 11:38:41 +10:00
Roman Rizzi 5c699e4384
DEV: Pass messageId as a dynamic segment instead of a query param (#20013)
* DEV: Rnemae channel path to just c

Also swap the channel id and channel slug params to be consistent with core.

* linting

* channel_path

* Drop slugify helper and channel route without slug

* Request slug and route models through the channel model if possible

* DEV: Pass messageId as a dynamic segment instead of a query param

* Ensure change is backwards-compatible

* drop query param from oneboxes

* Correctly extract channelId from routes

* Better route organization using siblings for regular and near-message

* Ensures sessions are unique even when using parallelism

* prevents didReceiveAttrs to clear input mid test

* we disable animations in capybara so sometimes the message was barely showing

* adds wait

* ensures finished loading

* is it causing more harm than good?

* this check is slowing things for no reason

* actually target the button

* more resilient select chat message

* apply similar fix to bookmark

* fix

---------

Co-authored-by: Joffrey JAFFEUX <j.jaffeux@gmail.com>
2023-02-01 12:39:23 -03:00
David Taylor eee97ad29a
DEV: Patch capybara to ignore client-triggered errors (#19972)
In dev/prod, these are absorbed by unicorn. Most commonly, they occur when a client interrupts a message-bus long-polling request.

Also reverts the EPIPE workaround introduced in 011c9b9973
2023-01-24 11:07:29 +00:00
Martin Brennan b4b8b03461
DEV: Add option to disable rspec diff truncation ENV var (#19861)
Sometimes you may have a large string or object that you are comparing
with some expectation, and you want to see the full diff between actual
and expected without rspec truncating 90% of the diff. Setting the
max_formatted_output_length to nil disables this truncation completely.

c.f. https://www.rubydoc.info/gems/rspec-expectations/RSpec/Expectations/Configuration#max_formatted_output_length=-instance_method

Use `RSPEC_DISABLE_DIFF_TRUNCATION=1` to disable this.
2023-01-13 13:31:28 +10:00
David Taylor cb932d6ee1
DEV: Apply syntax_tree formatting to `spec/*` 2023-01-09 11:49:28 +00:00
Martin Brennan 41acabad19
DEV: Change system spec JS log level to SEVERE by default (#19757)
Having this set to ALL pollutes the JS system spec
logs with a bunch of unnecessary noise like this:

> "PresenceChannel '/chat-user/core/1' dropped message (received 315, expecting 246), resyncing..."

Or:

> "DEPRECATION: The \u003Cdiscourse@component:plugin-connector::ember1112>#save computed property was just overridden. This removes the computed property and replaces it with a plain value, and has been deprecated.

Now, we will only log errors. To configure this set
the `SELENIUM_BROWSER_LOG_LEVEL` env var.
2023-01-05 16:41:22 +10:00