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
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.
Why this change?
This is abit of a trial and error but we're starting to see selenium
session not created errors on CI. One of the reason for this is that the
system has run out of resources to create a new tab.
This commit reduces the number of parallel test processors in an attempt
to increase the amount of resources available to each test process and
hopefully lead to more stable CI system tests.
This reverts commit 865f7a9852.
The flakiness that we have been seeing and fixing on CI were not related
to system resource problems. Therefore, we can bump this up back to 5.
Why is this change required?
We've been seeing flaky tests due to server errors on CI but are unable
to debug it because we do not log any of the errors. This change gives
us a fighting chance the next time we encounter a server error during
system test runs.
See
https://github.com/discourse/discourse/actions/runs/5459248864/jobs/9935049920?pr=22424
for an example of server errors encountered during system tests.
Using the runtime information, we will be able to more efficiently group
the test files across the test processes hence leading to better
utilization of resources.
Using the runtime information, we will be able to more efficiently group
the test files across the test processes hence leading to better
utilization of resources.
4dd053a69c addressed most of the
instability we were seeing with system tests on CI and locally. Let's
try pushing the number of parallel processes up to squeeze as much time
savings as possible from the runner.
We're running on pretty crappy hardware on Github's CI and this has an
impact on the stability of our system tests on CI. Therefore, we are
bumping `CABPYARA_DEFAULT_MAX_WAIT_TIME` to 10 seconds to account for
the less than ideal hardware we're running the system tests on.
This change trades off speed for stability but speed is already bad on
CI so stability is more important for our case.
## How does this work?
Any time a lint rule is added or changed, you can run `yarn lint:fix` to handle all the auto-fixable situations.
But not all lints are auto-fixable -- for those, lint-to-the-future has tooling to automatically ignore present violations.
An alias has been added for lint-to-the-future to ignore new violations, `yarn lttf:ignore`.
The command will add lint-ignore declarations throughout all the files with present violations, which should then be committed.
An excerpt from lint-to-the-future's [README](https://github.com/mansona/lint-to-the-future#lint-to-the-future-dashboard):
> The point of Lint to the Future is to allow you to progressively update your codebase using new lint rules without overwhelming you with the task. You can easily ignore lint rules using project-based ignores in your config files but that doesn't prevent you from making the same errors in new files.
> We chose to do the ignores on a file basis as it is a perfect balance and it means that the tracking/graphing aspects of Lint to the Future provide you with achievable goals, especially in large codebases.
## How do I view progress?
lint-to-the-future provides graphs of violations-over-time per lint rule in a dashboard format, so we can track how well we're doing at cleaning up the violations.
To view the dashboard locally, run `yarn lint-progress` and visit `http://localhost:8084` (or whatever the port it chose, as it will choose a new port if 8084 is preoccupied)
Also there is a `list` command which shows a JSON object of:
```ts
{
[date: string]: { // yyyy-mm-dd
[pluginName: string]: {
[fileName: string]: string[]; // list of files with violations
}
}
}
```
```bash
yarn lint-to-the-future list --stdout
```
## What about lint-todo?
Lint todo is another system available for both eslint and ember-template-lint that _forces_ folks to "leave things better than they found them" by being transparent / line-specific ignoring of violations.
It was decided that for _this_ project, it made more sense, and would be less disruptive to new contributors to have the ignore declarations explicitly defined in each file (whereas in lint-todo, they are hidden).
To effectively use lint-todo, a whole team needs to agree to the workflow, and in open source, we want "just anyway" to be able to contribute, and throwing surprises at them can deter contributions.
In production, `eager_load=true`. This sometimes leads to boot errors which are not present in dev/test environments. Running `zeitwerk:check` in CI will help us to pick up on any errors early.
This commit also introduces a `DISCOURSE_ZEITWERK_EAGER_LOAD` environment variable to make it easier to toggle the behaviour when developing locally.
This is a combined work of Martin Brennan, Loïc Guitaut, and Joffrey Jaffeux.
---
This commit implements a base service object when working in chat. The documentation is available at https://discourse.github.io/discourse/chat/backend/Chat/Service.html
Generating documentation has been made as part of this commit with a bigger goal in mind of generally making it easier to dive into the chat project.
Working with services generally involves 3 parts:
- The service object itself, which is a series of steps where few of them are specialized (model, transaction, policy)
```ruby
class UpdateAge
include Chat::Service::Base
model :user, :fetch_user
policy :can_see_user
contract
step :update_age
class Contract
attribute :age, :integer
end
def fetch_user(user_id:, **)
User.find_by(id: user_id)
end
def can_see_user(guardian:, **)
guardian.can_see_user(user)
end
def update_age(age:, **)
user.update!(age: age)
end
end
```
- The `with_service` controller helper, handling success and failure of the service within a service and making easy to return proper response to it from the controller
```ruby
def update
with_service(UpdateAge) do
on_success { render_serialized(result.user, BasicUserSerializer, root: "user") }
end
end
```
- Rspec matchers and steps inspector, improving the dev experience while creating specs for a service
```ruby
RSpec.describe(UpdateAge) do
subject(:result) do
described_class.call(guardian: guardian, user_id: user.id, age: age)
end
fab!(:user) { Fabricate(:user) }
fab!(:current_user) { Fabricate(:admin) }
let(:guardian) { Guardian.new(current_user) }
let(:age) { 1 }
it { expect(user.reload.age).to eq(age) }
end
```
Note in case of unexpected failure in your spec, the output will give all the relevant information:
```
1) UpdateAge when no channel_id is given is expected to fail to find a model named 'user'
Failure/Error: it { is_expected.to fail_to_find_a_model(:user) }
Expected model 'foo' (key: 'result.model.user') was not found in the result object.
[1/4] [model] 'user' ❌
[2/4] [policy] 'can_see_user'
[3/4] [contract] 'default'
[4/4] [step] 'update_age'
/Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/update_age.rb:32:in `fetch_user': missing keyword: :user_id (ArgumentError)
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:202:in `instance_exec'
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:202:in `call'
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:219:in `call'
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:417:in `block in run!'
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:417:in `each'
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:417:in `run!'
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:411:in `run'
from <internal:kernel>:90:in `tap'
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:302:in `call'
from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/spec/services/update_age_spec.rb:15:in `block (3 levels) in <main>'
```
This broke because of directory ownership errors during `git ls-files`. This commit fixes the permissions and adds bash flags so that those kind of errors will blow up the step in future.
The `git` version in our discourse_test docker image was recently updated to include a permissions check before running any git commands. For this to pass, the owner of the discourse directory needs to match the user running any git commands.
Under GitHub actions, by default the working directory is created with uid=1000 as the owner. We run all our tests as `root`, so this mismatch causes git to raise the permissions error. We can't switch to run the entire workflow as the `discourse (uid=1000)` user because our discourse_test image is not configured to allow `discourse` access to postgres/redis directories. For now, this commit updates the working directory's owner to match the user running the workflow.
Note this commit also slightly changes internal API: channel instead of getChannel and updateCurrentUserChannelNotificationsSettings instead of updateCurrentUserChatChannelNotificationsSettings.
Also destroyChannel takes a second param which is the name confirmation instead of an optional object containing this confirmation. This is to enforce the fact that it's required.
In the future a top level jsdoc config file could be used instead of the hack tempfile, but while it's only an experiment for chat, it's probably good enough.
This commit introduces the necessary gems and config, but adds all our ruby code directories to the `--ignore-files` list.
Future commits will apply syntax_tree to parts of the codebase, removing the ignore patterns as we go
Our working theory is that system tests on Github run on much less
powerful hardware as compared to running the tests on our work machines.
Hopefully, increasing the wait time now will help reduce some flakes
that we're seeing on Github.
These screenshots are located at paths like:
/__w/discourse/discourse/tmp/capybara/failures_r_spec_example_groups_quoting_chat_message_transcripts_copying_quote_transcripts_with_the_clipboard_quotes_multiple_chat_messages_into_a_topic_134.png
not /tmp/screenshots. This should fix the issue. Also makes plugin system specs
use documentation format and profile.
This commit introduces rails system tests run with chromedriver, selenium,
and headless chrome to our testing toolbox.
We use the `webdrivers` gem and `selenium-webdriver` which is what
the latest Rails uses so the tests run locally and in CI out of the box.
You can use `SELENIUM_VERBOSE_DRIVER_LOGS=1` to show extra
verbose logs of what selenium is doing to communicate with the system
tests.
By default JS logs are verbose so errors from JS are shown when
running system tests, you can disable this with
`SELENIUM_DISABLE_VERBOSE_JS_LOGS=1`
You can use `SELENIUM_HEADLESS=0` to run the system
tests inside a chrome browser instead of headless, which can be useful to debug things
and see what the spec sees. See note above about `bin/ember-cli` to avoid
surprises.
I have modified `bin/turbo_rspec` to exclude `spec/system` by default,
support for parallel system specs is a little shaky right now and we don't
want them slowing down the turbo by default either.
### PageObjects and System Tests
To make querying and inspecting parts of the page easier
and more reusable inbetween system tests, we are using the
concept of [PageObjects](https://www.selenium.dev/documentation/test_practices/encouraged/page_object_models/) in
our system tests. A "Page" here is generally corresponds to
an overarching ember route, e.g. "Topic" for `/t/324345/some-topic`,
and this contains logic for querying components within the topic
such as "Posts".
I have also split "Modals" into their own entity. Further down the
line we may want to explore creating independent "Component"
contexts.
Capybara DSL should be included in each PageObject class,
reference for this can be found at https://rubydoc.info/github/teamcapybara/capybara/master#the-dsl
For system tests, since they are so slow, we want to focus on
the "happy path" and not do every different possible context
and branch check using them. They are meant to be overarching
tests that check a number of things are correct using the full stack
from JS and ember to rails to ruby and then the database.
### CI Setup
Whenever a system spec fails, a screenshot
is taken and a build artifact is produced _after the entire CI run is complete_,
which can be downloaded from the Actions UI in the repo.
Most importantly, a step to build the Ember app using Ember CLI
is needed, otherwise the JS assets cannot be found by capybara:
```
- name: Build Ember CLI
run: bin/ember-cli --build
```
A new `--build` argument has been added to `bin/ember-cli` for this
case, which is not needed locally if you already have the discourse
rails server running via `bin/ember-cli -u` since the whole server is built and
set up by default.
Co-authored-by: David Taylor <david@taylorhq.com>
Both versions are used with `--headless`, so labelling one "Firefox" and the other "Firefox Headless" doesn't really make sense. Evergreen / ESR are better descriptions.
We added `always()` on some steps so that they run even if previous steps fail. That helps give us a picture of all failures in one run, rather than having to re-run the workflow after fixing the first failure.
However, when we explicitly cancel a job, we should skip running these steps. `!cancelled()` is a better substitute for `always()` in this case.
When `EMBER_CLI_PLUGIN_ASSETS=1`, plugin application JS will be compiled via Ember CLI. In this mode, the existing `register_asset` API will cause any registered JS files to be made available in `/plugins/{plugin-name}_extra.js`. These 'extra' files will be loaded immediately after the plugin app JS file, so this should not affect functionality.
Plugin compilation in Ember CLI is implemented as an addon, similar to the existing 'admin' addon. We bypass the normal Ember CLI compilation process (which would add the JS to the main app bundle), and reroute the addon Broccoli tree into a separate JS file per-plugin. Previously, Sprockets would add compiled templates directly to `Ember.TEMPLATES`. Under Ember CLI, they are compiled into es6 modules. Some new logic in `discourse-boot.js` takes care of remapping the new module names into the old-style `Ember.TEMPLATES`.
This change has been designed to be a like-for-like replacement of the old plugin compilation system, so we do not expect any breakage. Even so, the environment variable flag will allow us to test this in a range of environments before enabling it by default.
A manual silence implementation is added for the build-time `ember-glimmer.link-to.positional-arguments` deprecation while we work on a better story for plugins.
Each test chunk takes about 10 minutes, so those timeouts can be decreased from 20 to 15.
And there are three of those chunks so total can be a bit over 30 minutes, hence the bump to 35.
Anyone still using `EMBER_CLI_PROD_ASSETS=0` in development or production will be gracefully switched to Ember CLI. In development, a repeated message will be logged to STDERR.
Similarly, passing `QUNIT_EMBER_CLI=0` to the qunit rake task will now do nothing. A warning will be printed, and ember-cli mode will be used. Note that we've chosen not to fail the task, so that existing plugin/theme CI jobs don't immediately start failing. We may switch to a hard fail in the coming days/weeks.
`run-qunit.js` does not expect QUnit tests to start automatically but
our wizard QUnit setup did not respect the `qunit_disable_auto_start`
URL param. Hence, tests would start running automatically and when a
subsequent `QUnit.start()` function call is made, we ended up getting a
`QUnit.start cannot be called inside a test context.` error.
This error can be consistently reproduced in the `discourse:discourse_test` container but not in
the local development environment. I do not know why and did not feel
like it is important at this point in time to know why.
Previously, if Core QUnit 1 failed, then QUnit 2/3 wouldn't even be attempted. When dealing with multiple failures, this can make the feedback cycle. Setting `if: always()` ensures that the steps run regardless of any earlier failures. This is the same approach we take in the linting workflow.
We have 3 branches which we care about, `main`, `beta` and `stable`.
However, each of this branch has different compatibilties with plugins
and we want to respect that.
This reverts commit f43bba8d59.
Adding randomness has introduced a lot of flakiness in our ember-cli tests. We should fix those issues at the source. However, given the upcoming stable release, this randomness has been reverted so that the stable release includes a stable test suite. Having a stable test suite on stable will make backporting future commits much easier.
- Move ember-cli tests into the main test workflow, so they're listed alongside other tests
- Remove the 'experimental' label
- Add the 'legacy' label to old-style qunit tests
- Add core-plugin EmberCLI tests
- Add scaffolding for all-plugin EmberCLI tests, but disable in matrix for now
The discourse base image already contains a postgres installation, so pulling a separate postgres image is a little wasteful. Using the copy of Postgres in the discourse image saves about 20 seconds on every GitHub actions run.
This commit sets up Postgres with a few performance-improving flags, which we were already using for the `rake docker:test` task (used on our internal CI system).
A cached database (and its uploads) will only be used if the current run has exactly the same set of migration files. Otherwise, the database will be migrated from scratch
This saves approximately 75s on the core backend specs and 45s on other runs.
`bin/rake annotate` is an alias of `bin/annotate --models`
`bin/rake annotate:clean` generates annotations by using a temporary, freshly migrated database. This should help us to produce more consistent annotations, even if development databases have been polluted by plugin migrations.
A GitHub actions task is also added which generates annotations on a clean database, and raises an error if they differ from the committed annotations.
Over the years we accrued many spelling mistakes in the code base.
This PR attempts to fix spelling mistakes and typos in all areas of the code that are extremely safe to change
- comments
- test descriptions
- other low risk areas
Includes:
* DEV: Remove external plugin linting (that's covered by CI in their repositories)
* DEV: Move lint stages to a separate workflow (partial de-`if`-ication of workflows)
* DEV: Run CI on `main` branch too
* DEV: Update postgres to 13
* DEV: Update redis to 6.x
Other changes:
* DEV: Remove matrix.os
* DEV: Remove env.BUILD_TYPE
* DEV: Remove env.TARGET
* DEV: Rename `build_types` config option to `build_type`
* DEV: Lowercase `target` and `build_type` names
* DEV: Rename `ci` to `tests`
* DEV: Rename `lint` to `linting`
* DEV: Lower the wizard qunit timeout (30 min -> 10)
* DEV: Ruby version is no longer configurable
* DEV: Run plugin tests only in the `plugins` target
* DEV: Use binstubs where applicable
* DEV: We don't open PRs to `tests-passed`
Using our testing Docker image (`discourse/discourse_test:release`) allows us to drop "Update imagemagick" step which shaves ~10 minutes from all runs.
Dependency on gifsicle, allow_animated_avatars and allow_animated_thumbnails
site settings were all removed. Animated GIF images are still allowed, but
the generated optimized images are no longer animated for those (which were
used for avatars and thumbnails).
The added 'animated' is populated by extracting information using FastImage.
This field was used to selectively reoptimize old animations. This process
happens in the background.
This is where they should be as far as ember is concerned. Note this is
a huge commit and we should be really careful everything continues to
work properly.
* DEV: Don't lint core files when target == plugins
* Prettier the plugin/*.js files
* Update eslintignore/prettierignore
* Add eslint-plugin-ember and eslint-plugin-node
* Properly lint all js files in all plugins
* LINT: run prettier on test/*.js files
* DEV: Run prettier checks on test/*.js files
* ESLint plugins' assets/javascripts and test/javascripts directories only
Moves the most important checks into a linter. It gets executed by Lefthook as well as the docker rake task and Github actions. Doing those checks in rspec takes too long and it produces errors when the discourse:test Docker image contains old, invalid locale files.
* DEV: Update bundler in GitHub Actions CI
* DEV: Fix bundler deprecation warning
Fixes the following two deprecation warnings:
```
[DEPRECATED] The `--deployment` flag is deprecated because it relies on being remembered across bundler invocations, which bundler will no longer do in future versions. Instead please use `bundle config set deployment 'true'`, and stop using this flag
```
```
[DEPRECATED] The `--without` flag is deprecated because it relies on being remembered across bundler invocations, which bundler will no longer do in future versions. Instead please use `bundle config set without 'development'`, and stop using this flag
```
* DEV: The default `retry` value is already `3`
See https://bundler.io/v2.0/man/bundle-config.1.html:
> retry (BUNDLE_RETRY): The number of times to retry failed network requests. Defaults to 3.
* DEV: `&& \` isn't required in multiline `step.run`
Steps use the fail-fast strategy.
* DEV: Use multiline `step.run` where possible
