What is the problem?
We have a need to disable pitchfork via the `RUN_PITCHFORK` env.
However, `RUN_PITCHFORK` is checked for existence/non-emptiness in both
`bin/rails` and `config/unicorn_launcher`. This means setting
`RUN_PITCHFORK=0` enables pitchfork, which is counterintuitive.
What is the solution?
Change the checks to explicitly test for the value `"1"`. Now only
`RUN_PITCHFORK=1` enables pitchfork, while `0` or unset uses unicorn.
What is the problem?
Running `bin/lint` or `bin/lint --fix` without arguments did not run
syntax_tree checks on Ruby files. The `lints` and `fix-all` hooks in
`lefthook.yml` were missing syntax_tree commands, so Ruby formatting
issues would not be detected or fixed.
Additionally, users with `LEFTHOOK=0` in their environment would have
`bin/lint` silently skip all checks.
What is the solution?
Add `syntax_tree` commands to both the `lints` hook (for checking) and
`fix-all` hook (for auto-fixing) in `lefthook.yml`. These use
`{all_files}` with glob filtering to check all tracked Ruby files.
Also update `LefthookLinter#exec_lefthook` in `bin/lint` to explicitly
set `LEFTHOOK=1` environment variable, ensuring lefthook runs regardless
of the user's environment settings.
When using ember exam, we should pass the seed as a CLI argument. It
should only be a query parameter for our custom non-ember-exam test
runs.
Also updates bin/qunit to pass a shorter alphanumeric seed, instead of a
long integer.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: davidtaylorhq <6270921+davidtaylorhq@users.noreply.github.com>
Co-authored-by: Jarek Radosz <jradosz@gmail.com>
This amends lefthook to use the more reliable doublestar globber:
https://github.com/evilmartians/lefthook/blob/master/docs/mdbook/configuration/glob_matcher.md
it also cleans up the config so we can properly run and filter.
Notably ember linter explodes if you pass it files your are not meant to
lint.
The current fix has bin/lint read the config from lefthook and filter
out the files that need no linting. `--file` overrides the glob so we
can not pass in files unfiltered.
This also simplifies some of the linters internals for easier
maintenance
This PR implements stricter deprecation handling that enforces
deprecation-free tests for core and preinstalled plugins, while allowing
custom (non-preinstalled) plugins and themes to have deprecations
without causing test failures.
### Key Changes
#### CI Workflow Improvements
- Split plugin system tests into separate CI targets: `core-plugins`,
`official-plugins`, and `chat`
- Enhance `bin/turbo_rspec` to accept comma-separated exclude patterns
via `--exclude-pattern`
- Simplify workflow configuration with default `shell: bash` and
consolidated environment variables
#### Plugin Classification & Detection
- Centralize official plugins list in `config/official_plugins.json` for
unified backend and frontend access
- Detect preinstalled plugins by checking for absence of `.git`
directory
- Add `isOfficial` and `isPreinstalled` metadata flags to plugin info
- Add `data-preinstalled` and `data-official` attributes to all plugin
and theme script tags for runtime identification
#### Deprecation Source Tracking
- Track deprecation sources (core, plugin, or theme) through template
map and resolver to attribute deprecations correctly
- Improve `source-identifier.js` to detect admin UI plugin files in both
development and production environments
- Add source information to deprecation messages for better debugging
#### Test Infrastructure
- Modify `raise-on-deprecation` test helper to skip errors for custom
(non-preinstalled) plugins and themes
- Add `EMBER_RAISE_ON_DEPRECATION` environment variable to control
deprecation throwing behavior in Rails tests
- Automatically set `EMBER_RAISE_ON_DEPRECATION` for core and
preinstalled plugin/theme specs in `rails_helper.rb`
- Improve deprecation summary output for system specs with test/spec
origin tracking
#### Deprecation Workflow Enhancements
- Add `dont-throw` handler for selective deprecation bypassing in test
fixtures without raising errors
- Add `dont-count` handler for preventing deprecation counting in
specific scenarios (e.g., test fixtures)
#### Deprecation Fixes
- Fix pending deprecations across core plugins (chat, data-explorer,
discourse-subscriptions, gamification, house-ads, reactions,
rss-polling, styleguide)
- Update import paths and remove deprecated patterns
- Migrate deprecated Handlebars templates to JavaScript API
### Testing Strategy
With these changes:
- **Core and preinstalled plugins** must pass all tests without any
deprecations
- **Custom plugins and themes** can have deprecations without failing
tests
- Test fixtures can use `dont-throw` and `dont-count` handlers when
testing deprecation behavior itself
- System specs automatically configure deprecation enforcement based on
test file location
---------
Co-authored-by: David Taylor <david@taylorhq.com>
Co-authored-by: Jarek Radosz <jradosz@gmail.com>
When the rails server is already running, attempting to start
it again was giving an ArgumentError and outputting the backtrace.
When you are working locally, all you want here is the PID so you
can kill it.
I altered the error message to this in this case:
```
Unicorn is already running!
kill -9 150044 to stop the existing unicorn master process, then try again.
```
So you can easily copy and paste the command to stop the existing
server.
The old error:
<img width="1711" height="243" alt="image"
src="https://github.com/user-attachments/assets/18d5b25b-5da9-45b0-941e-6ee353423cf3"
/>
Theme qunit tests are run in RAILS_ENV=development, which has
LOAD_PLUGINS enabled by default. We should allow that default to take
effect, without overriding it with `LOAD_PLUGINS=0`.
This is essential for themes which depend on plugins for their tests to
work.
- Switches to typescript 'composite project' mode for improved
organization & performance
- Generates declarations from core modules, and outputs to a new
`@discourse/types` package
- Introduces tsconfigs for core plugins, which leans on that new
`@discourse/types` package
- In future, we plan to publish this discourse-types package for other
themes/plugins to use
The motivation at this stage is primarily to improve editor
autocompletion. We provide absolutely no guarantees about the accuracy
or stability of the generated types.
For now, some issues have been resolved by adding manual `@type`
declarations.
In particular, extensionless imports of `.gjs` files are broken when
running in composite-project mode, so in those cases we have to add
manual `@type {import("./the-file.gjs").default}` declarations.
Unfortunately this failure does not show up in IDEs. We hope to get this
resolved soon.
---------
Co-authored-by: David Taylor <david@taylorhq.com>
Co-authored-by: Chris Manson <chris@manson.ie>
This PR adds Pitchfork, as we want to move away from Unicorn ultimately.
Unicorn still boots by default, so there should be no disruption for
anyone.
To use Pitchfork instead of Unicorn, the `RUN_PITCHFORK` environment
variable must be set.
This will make `bin/rails s` and `config/unicorn_launcher` boot
Pitchfork. `unicorn_launcher` was patched because that way we can easily
switch between Unicorn and Pitchfork without having to change too many
things on the infra side.
The upgrader from the `docker_manager` plugin doesn’t work yet with
Pitchfork. This will be addressed in a future PR.
- Do a simple check to see whether JS deps are out-of-date before
running `pnpm install`. This saves about 1 second on my machine
- Remove `playwright install` from the critical path. Instead, add a
`pnpm playwright-install` shortcut, and improve the error which system
specs throw when playwright is out of date. This saves about 0.7s on my
machine.
This change seeks to improve the reliability of our system tests by
resolving the lack of consistency in the state of the client side
application between steps in a system test. This is achieved by patching
various action methods in `Capybara::Playwright::Node` and
`Capybara::Playwright::Browser` so that the methods execute an async
JavaScript function on the client side that waits for the client side
application to reach a settled state. A settled state is currently
defined as:
1. No inflight ajax requests. (_messageBus and presence requests are
excluded_)
2. 2 event cycles of the Javascript event loop has happened for for all
"click", "input", "mousedown", "keydown", "focusin", "focusout",
"touchstart", "change", "resize", "scroll" DOM events that fired.
For debugging purposes, a `--debug-client-settled` CLI flag has been
added to `bin/rspec`. When used, detailed debugging information will be
printed to the browser's console as well as to `stdout` of the
`bin/rspec` process.
This change was inspired by
https://evilmartians.com/chronicles/flaky-tests-be-gone-long-lasting-relief-chronic-ci-retry-irritation
and the https://github.com/makandra/capybara-lockstep rubygem.
1. Use relative paths instead of absolute. This will make the commands
portable to other machines, and matches the out-the-box behavior of the
regular rspec command
2. Strip out some 'infrastructure' ENV and params from the
rerun_command, so that it's easier to copy/paste and run locally
Shortened AI agent file, so we conserve tokens
Use symlinks for all agent files where needed to avoid round trips to
llm
Added config for Open AI Codex / Gemini and Cursor
- Migrated from annotate to annotaterb as the former is not maintained
anymore.
- Dropped our `fast_pluck` patch as the default `pluck` implementation
seems now faster.
When more than 5 files are run via `turbo_rspec`, we'll show the count.
When less than 5 files, we'll list them.
This is slightly better DX as it avoids spamming the console with
unecessary information.
Users won't be able to use video locally but we think it's not necessary
for now. This could also cause permissions errors on CI as deps require
sudo to be installed.
We have had multiple issues while using the chrome channel:
- super slow start of the spec, I can confirm I don't have it on
chromium
- more risk of different failures between local dev machines and CI
There was a test not working on chromium, as it doesn't have mp4 codecs
out of the box. I changed the spec to use webm instead and it's now
working correctly.
This commit also fixes a bug with the video/trace paths which were
incorrect, and also makes another test less flakey with uploads event
though Im not very clear on what is going on here, we need to dig this.
We now need to figure out a way to have this on the test image, but this
is for now a better solution.
This commit is replacing the system specs driver (selenium) by
Playwright: https://playwright.dev/
We are still using Capybara to write the specs but they will now be run
by Playwright. To achieve this we are using the non official ruby
driver: https://github.com/YusukeIwaki/capybara-playwright-driver
### Notable changes
- `CHROME_DEV_TOOLS` has been removed, it's not working well with
playwright use `pause_test` and inspect browser for now.
- `fill_in` is not generating key events in playwright, use `send_keys`
if you need this.
### New spec options
#### trace
Allows to capture a trace in a zip file which you can load at
https://trace.playwright.dev or locally through `npx playwright
show-trace /path/to/trace.zip`
_Example usage:_
```ruby
it "shows bar", trace: true do
visit("/")
find(".foo").click
expect(page).to have_css(".bar")
end
```
#### video
Allows to capture a video of your spec.
_Example usage:_
```ruby
it "shows bar", video: true do
visit("/")
find(".foo").click
expect(page).to have_css(".bar")
end
```
### New env variable
#### PLAYWRIGHT_SLOW_MO_MS
Allow to force playwright to wait DURATION (in ms) at each action.
_Example usage:_
```
PLAYWRIGHT_SLOW_MO_MS=1000 rspec foo_spec.rb
```
#### PLAYWRIGHT_HEADLESS
Allow to be in headless mode or not. Default will be headless.
_Example usage:_
```
PLAYWRIGHT_HEADLESS=0 rspec foo_spec.rb # will show the browser
```
### New helpers
#### with_logs
Allows to access the browser logs and check if something specific has
been logged.
_Example usage:_
```ruby
with_logs do |logger|
# do something
expect(logger.logs.map { |log| log[:message] }).to include("foo")
end
```
#### add_cookie
Allows to add a cookie on the browser session.
_Example usage:_
```ruby
add_cookie(name: "destination_url", value: "/new")
```
#### get_style
Get the property style value of an element.
_Example usage:_
```ruby
expect(get_style(find(".foo"), "height")).to eq("200px")
```
#### get_rgb_color
Get the rgb color of an element.
_Example usage:_
```ruby
expect(get_rgb_color(find("html"), "backgroundColor")).to eq("rgb(170, 51, 159)")
```
This commit adds a new `--exclude-pattern` CLI option which supports
excluding files using unix style pattern matching.
This is to simplify a couple of situations:
1. Running `bin/turbo_rspec plugin/spec` but we want to exclude all
tests in the `plugin/spec/system` folder.
Example: `bin/turbo_rspec --exclude-pattern="*spec/system*"
plugin/spec`
2. Running `bin/turbo_rspec plugin/*/spec/system` but we want to exclude
tests for a particular plugin.
Example: `bin/turbo_rspec --exclude-pattern="plugins/chat/*"
plugin/*/spec/system`
When passing a `plugins/...` path to bin/rspec, it's reasonable to assume that the developer wants the tests to be run with plugins loaded. This commit automatically takes care of that.
If `LOAD_PLUGINS` is explicitly specified, then that value will always be used.
This mimics the logic we already have for database migrations in the `bin/rake` stub
- Uses a more appropriate image, with immutable tag (so update prompts work correctly)
- Updates port forwarding
- Improves mount setup (inc. persistant PG/Redis when rebuilding)
- Fixes ember-cli live reload
- Automatically configures VSCode & extensions
Our sprockets-based locale pipeline correctly `depends_on` client.*.yml files which already exist. But sprockets does not provide a way to watch for the creation of new files matching a certain path. This commit works around that limitation by adding the set of client.*.yml files to our global cache-clearing logic
When attempting to [set up the Docker development environment](https://meta.discourse.org/t/install-discourse-for-development-using-docker/102009), I was seeing the following error when running `d/boot_dev --init`:
```
ActiveRecord::NoDatabaseError: We could not find your database: discourse_development. Available database configurations can be found in config/database.yml. (ActiveRecord::NoDatabaseError)
```
Running `db:create` before the `db:migrate` fixed this issue for me. It appears to be safe to run `db:create` even if the database already exists, running `d/rake db:create` locally shows an info message at the database already exists, but doesn't exit with an error.
This will bring significant improvements to install speed & storage requirements. For information on how it may affect you, see https://meta.discourse.org/t/324521
This commit:
- removes the `yarn.lock` and replaces with `pnpm-lock.yaml`
- updates workspaces to pnpm format
- adjusts package dependencies to work with pnpm's stricter resolution strategy
- updates Rails app to load modules from more specific node_modules directories
- adds a `.pnpmfile` which automatically cleans up old yarn-managed `node_modules` directories
- updates various scripts to call `pnpm` instead of `yarn`
- updates patches to use pnpm's native patch system instead of patch-package
- adds a patch for licensee to support pnpm
`bin/ember-cli -u` buffers the output of ember-cli, which means progress is not shown clearly. This commit uses `TERM=dumb` to put ember-cli into a more simple mode, which outputs progress on separate lines. Also adds a `[ember-cli]` prefix to all output.
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.
Why this change?
Pull requests can introduce flaky tests into the mix and we do not want
to be hiing that during the pull request process. While this does mean
builds for PR will be less stable than the `main` branch without
retries, we do not foresee this to be a problem long term since the
monitoring of flaky tests on the `main` branch will mean that the number
of flaky tests will eventually be reduced.
What does this change do?
1. Introduce the `DISCOURSE_TURBO_RSPEC_RETRY_AND_LOG_FLAKY_TESTS` env
variable which will initialize `TurboTest::Runner` with the `retry_and_log_flaky_tests`
kwarg set to true when set.
2. Change the tests workflow run to set `DISCOURSE_TURBO_RSPEC_RETRY_AND_LOG_FLAKY_TESTS` only when
the build type is `backend` or `system` and the `github.ref_name` is
`main`.
What motivated this change?
Our builds on Github actions have been extremely flaky mostly due to system tests. This has led to a drop in confidence
in our test suite where our developers tend to assume that a failed job is due to a flaky system test. As a result, we
have had occurrences where changes that resulted in legitimate test failures are merged into the `main` branch because developers
assumed it was a flaky test.
What does this change do?
This change seeks to reduce the flakiness of our builds on Github Actions by automatically re-running RSpec tests once when
they fail. If a failed test passes subsequently in the re-run, we mark the test as flaky by logging it into a file on disk
which is then uploaded as an artifact of the Github workflow run. We understand that automatically re-runs will lead to
lower accuracy of our tests but we accept this as an acceptable trade-off since a fragile build has a much greater impact
on our developers' time. Internally, the Discourse development team will be running a service to fetch the flaky tests
which have been logged for internal monitoring.
How is the change implemented?
1. A `--retry-and-log-flaky-tests` CLI flag is added to the `bin/turbo_rspec` CLI which will then initialize `TurboTests::Runner`
with the `retry_and_log_flaky_tests` kwarg set to `true`.
2. When the `retry_and_log_flaky_tests` kwarg is set to `true` for `TurboTests::Runner`, we will register an additional
formatter `Flaky::FailuresLoggerFormatter` to the `TurboTests::Reporter` in the `TurboTests::Runner#run` method.
The `Flaky::FailuresLoggerFormatter` has a simple job of logging all failed examples to a file on disk when running all the
tests. The details of the failed example which are logged can be found in `TurboTests::Flaky::FailedExample.to_h`.
3. Once all the tests have been run once, we check the result for any failed examples and if there are, we read the file on
disk to fetch the `location_rerun_location` of the failed examples which is then used to run the tests in a new RSpec process.
In the rerun, we configure a `TurboTests::Flaky::FlakyDetectorFormatter` with RSpec which removes all failed examples from the log file on disk since those examples are not flaky tests. Note that if there are too many failed examples on the first run, we will deem the failures to likely not be due to flaky tests and not re-run the test failures. As of writing, the threshold of failed examples is set to 10. If there are more than 10 failed examples, we will not re-run the failures.
This commit introduces the scaffolding for us to easily switch between Ember 3.28 and Ember 5 on the `main` branch of Discourse. Unfortunately, there is no built-in system to apply this kind of flagging within yarn / ember-cli. There are projects like `ember-try` which are designed for running against multiple version of a dependency, but they do not allow us to 'lock' dependency/sub-dependency versions, and are therefore unsuitable for our use in production.
Instead, we will be maintaining two root `package.json` files, and two `yarn.lock` files. For ember-3, they remain as-is. For ember5, we use a yarn 'resolution' to override the version for ember-source across the entire yarn workspace.
To allow for easy switching with minimal diff against the repository, `package.json` and `yarn.lock` are symlinks which point to `package-ember3.json` and `yarn-ember3.lock` by default. To switch to Ember 5, we can run `script/switch ember version 5` to update the symlinks to point to `package-ember5.json` and `package-ember3.json` respectively. In production, and when using `bin/ember-cli` for development, the ember version can also be upgraded using the `EMBER_VERSION=5` environment variable.
When making changes to dependencies, these should be made against the default `ember3` versions, and then `script/regen_ember_5_lockfile` should be used to regenerate `yarn-ember5.lock` accordingly. A new 'Ember Version Lockfiles' GitHub workflow will automate this process on Dependabot PRs.
When running a local environment against Ember 5, the two symlink changes will show up as git diffs. To avoid us accidentally committing/pushing that change, another GitHub workflow is introduced which checks the default Ember version and raises an error if it is greater than v3.
Supporting two ember versions simultaneously obviously carries significant overhead, so our aim will be to get themes/plugins updated as quickly as possible, and then drop this flag.
Why this change?
Before this change, running `d/boot_dev` does not run `bundle install`
and `yarn install` unless the `--init` option is specified. However,
this does not make sense because the user will end up having to run
`d/bundle install` and `d/yarn install` manually after if the `--init`
option is not used. We can simplify this by just always running `bundle
install` and `yarn install` whenever `d/boot_dev` is used.
What does this change do?
This change changes the `d/boot_dev` script to always run `bundle
install` and `yarn install`.
We were already running `yarn install` for the app directory before booting ember-cli via `bin/ember-cli`. This commit extends that so that it also runs `yarn install` for the root of the repository. In the long-term we hope to combine these packages, but for now they are separate.
The post-install hook for the root package is also updated to pass through the `-s` flag. Previously, we only passed through the `--frozen-lockfile` flag.
We have been discussing potentially adding a Procfile to help launch the application. As part of the discussions, @tgxworld also mentioned it'd be nice to have a command to launch the app in development.
This allows us to encode how to start the app in code and ship that in the repo, rather than having to rely on external documentation.
Newer installs of Rails come with a bin/dev script for exactly this purpose. This change adds one in retroactively for us.
The script currently runs bin/ember-cli -u, which is the canonical way of starting the app for development according to documentation. I know not everyone uses this, but the point of this PR is to add the script first, then we can iterate on the means of running the app. 🙂
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.
Alan Guo Xiang Tan
Sam
Martin Brennan
Loïc Guitaut
Régis Hanol
Joffrey JAFFEUX
Gary Pendergast
Mark VanLandingham
Daniel Waterworth
Ted Johansson