Skip to content

Added e2e tests for alerting #3570

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Added e2e tests for alerting #3570

wants to merge 23 commits into from

Conversation

skrech
Copy link
Contributor

@skrech skrech commented Jun 13, 2025

Description

This PR adds E2E tests for the alerting configuration. Since alerting configuration supports setting it from either environment variables or from the DB different setup is required for running the backend. This necessitates splitting the tests into two groups -- one for the case where environment variables are in place and no dynamic configuration is possible and the second one is when the config is read from the DB and can be modified by the user in real time using the Web API.
The former case can be considered as the default behavior and its tests are run along the rest of the parallel test suite.
The later case is protected by an env var ALERTING_DB_TESTS and has its own custom GitHub Actions job.

Added is a new dependency -- pg package which allows to directly interact with the Postgres DB in some of the alerting configuration tests since there is no other way to control the fixture by solely using the API.

Updated is the developer documentation on how to enable setting alerting configuration via the web API during development.

@skrech skrech added the test label Jun 13, 2025
@skrech skrech requested a review from nelsonkopliku June 19, 2025 13:36
@skrech skrech marked this pull request as ready for review June 19, 2025 13:36
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, @skrech!

CI changes are expected in this PR or a different one?

@skrech skrech force-pushed the kkanchev/alerting-settings-model branch from 211b572 to 742d238 Compare June 20, 2025 11:00
@skrech skrech force-pushed the alerting-e2e branch 3 times, most recently from 50324c2 to c29c9d5 Compare June 20, 2025 16:35
@skrech
Copy link
Contributor Author

skrech commented Jun 20, 2025

Added the CI changes. Instead of creating a new Job, though, I went for re-using the test-integration job. It's definition is generic enough and the only redundant step is starting the idp profile for docker compose (but this causes no harm for alerting).
I'd even suggest we rename the test-integration job to something more generic like e2e-parameterized, for example - effectively it takes all the steps necessary to parameterize e2e test -- setting cypress env-vars, modifying the compile-time elixir configuration and specifying what spec files to run. We can also add the docker compose profile in the matrix and then it would become truly universal.
I had to re-arrange settings.cy.js for better skip handling. It turned out in the case where we're using the test-integration job the subset of desired alerting tests are run along with the tests for suma, activity log, etc. To avoid that with as little change to existing files as possible, I decided to split the alerting tests involving DB interaction into their own file settings_integration.cy.js following the de facto convention established by sso_integration.cy.js. This works great, IMO apart from the somewhat misleading name -- settings_integration, there is no integration actually going on.
I also added a small protection mechanism to skip the enforced-from-env test if they are run against local development BE target currently using the DB for configuration.
With that, this PR seems to be complete. All CI checks are passing. If there is a consensus on leaving the SQL excerpt as is, then I think it's in a good shape to be merged.

Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, @skrech!

As mentioned privately I was confused by the fact that from the test setup it looks like the default behaviour is env var driven alerting settings, which I understand being the case for development.

However, if I recall correctly, for the user the default behaviour is database driven, right?

@skrech skrech force-pushed the kkanchev/alerting-settings-model branch from 9d7a75c to 15afa77 Compare June 24, 2025 08:07
Base automatically changed from kkanchev/alerting-settings-model to main June 24, 2025 08:26
Copy link
Contributor

@vicenteqa vicenteqa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks nice @skrech !

Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

@@ -242,6 +242,6 @@ defmodule Trento.MixProject do
end

defp get_version_from_file do
File.cwd!() |> Path.join("VERSION") |> File.read!() |> String.trim()
__DIR__ |> Path.join("VERSION") |> File.read!() |> String.trim()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What issues was the previous function usage causing?
Not that I am against the change, just to understand 😄

Copy link
Contributor Author

@skrech skrech Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good catch :) I even intend to leave this into its own commit when merging this.

It can't be seen from the diff of the PR but I tried very hard to actually start the Mix task from different current working directory (because Cypress doesn't run in the project root). And when specifying custom path to mix.exs, reading the file won't succeed because it can't find VERSION file in what happens to be the current directory. __DIR__ would work because it's bound to the location of the file being read, so very robust.

Unfortunately, specifying MIX_EXS and MIX_DEPS_PATH wasn't enough to make mix running from non-project-root succeed because it couldn't find mix.lock. That seems to be a bug, so I gave up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants