Skip to content

Verification / Detection follow-ups#5836

Merged
RobertJoonas merged 8 commits intomasterfrom
verification-detection-followups
Oct 27, 2025
Merged

Verification / Detection follow-ups#5836
RobertJoonas merged 8 commits intomasterfrom
verification-detection-followups

Conversation

@RobertJoonas
Copy link
Copy Markdown
Contributor

@RobertJoonas RobertJoonas commented Oct 27, 2025

Changes

  1. Consider internal check timeouts to be Browserless issues in both detection and verification
  2. Assert on captured sentry messages

Note: This PR makes the timeout_ms (that is currently fixed for each InstallationSupport.Check module) configurable on the CheckRunner level. This will also be useful for increasing detection timeout for the domain change flow.

Tests

  • Automated tests have been added

Changelog

  • This PR does not make a user-facing change

Documentation

  • This change does not need a documentation update

Dark mode

  • This PR does not change the UI

@RobertJoonas RobertJoonas force-pushed the verification-detection-followups branch from 03c3bd2 to 1436c0d Compare October 27, 2025 14:24
@RobertJoonas RobertJoonas force-pushed the verification-detection-followups branch from 1436c0d to e998ae5 Compare October 27, 2025 14:45
@RobertJoonas RobertJoonas marked this pull request as ready for review October 27, 2025 15:34
@RobertJoonas RobertJoonas changed the title Verification detection followups Verification / Detection follow-ups Oct 27, 2025
@RobertJoonas RobertJoonas requested a review from apata October 27, 2025 15:41
@@ -0,0 +1,188 @@
defmodule Plausible.InstallationSupport.Verification.ChecksObservabilityTest do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion, non-blocking: checks_test, checks_observability_test for both Verification and Detection have quite a bit of similar code, like stubbing, test sentry conf, run_checks, test scenarios.

I think we could cut down on this duplication a lot by doing the following:
In the main checks_test modules, we make sure that expected %Result's are achieved.
In o11y-related unit tests, we make sure that given certain %Result's, handle_result triggers certain logs, metrics etc

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair suggestion by all means, but I personally like how these tests do not worry about any intermediate results and focus solely on how a certain Browserless response gets handled. They're still fast, cheap and easy to follow.

@RobertJoonas RobertJoonas added this pull request to the merge queue Oct 27, 2025
Merged via the queue into master with commit 46f05d8 Oct 27, 2025
16 checks passed
@RobertJoonas RobertJoonas deleted the verification-detection-followups branch November 3, 2025 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants