Cover the failure paths the suite was missing - #143
Merged
Conversation
The sync action had a full broker-failure test matrix; its siblings did not. New coverage, each test pinned to a behavior that could regress silently: - Emitter: a broker error response is logged (with a body excerpt) without blocking the save; a 409 whose PATCH fallback also fails reports the second failure; DELETE tolerates 404 without logging; one failing connection does not block the next one. - Webhook: an all-invalid batch answers 422 with the validation messages (a permanent error the broker should not retry); a mixed batch answers 200 with per-outcome counts; an unconvertible geometry degrades to no geometry instead of failing the notification. - Publish/unpublish server-side failures: broker 400, missing Location, network failure (subscription_id stays nil in all three); unpublish 204 clears the id, broker 500 keeps it; proxied mode without a header token never calls the broker. - Federation panel: the old test named ..._is_404 asserted a login redirect (core behavior); the real guard is now tested: an authenticated non-member gets a 404 for an invisible issue and the broker is never queried. - Preview: a broker 200 with an unparsable body is a 422, not a 500. - HTML permission denial: preview and publish answer 403 without the permission, and publish never reaches the broker. Full suite: 355 runs, 1215 assertions, 0 failures.
There was a problem hiding this comment.
Pull request overview
Adds missing negative-path coverage across the test suite to ensure publish/unpublish, webhook ingestion, federation panel visibility, and broker/HTTP failure handling behave consistently and safely under error conditions (no production changes).
Changes:
- Expands emitter tests to cover non-success responses, PATCH-fallback failure logging, DELETE 404 tolerance, and per-connection isolation.
- Adds a publish/unpublish server-side failure matrix (broker error shapes, missing Location, network failure, proxied-without-token) plus authorization checks and preview 422 handling for unparsable broker bodies.
- Adds webhook contract tests for full-invalid batch 422 with errors, mixed batch 200 semantics, and geometry degradation; updates federation panel tests to correctly distinguish login redirect vs. authenticated 404 visibility guarding.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/unit/emitter_test.rb | Adds tests for broker error logging, PATCH fallback failures, DELETE 404 tolerance, and connection isolation. |
| test/functional/subscription_templates_controller_test.rb | Adds publish/unpublish failure matrix tests, preview 422 for unparsable bodies, and permission-based 403 coverage. |
| test/functional/subscription_issues_controller_test.rb | Adds webhook 422/200 contract tests and geometry degradation behavior coverage. |
| test/functional/federation_controller_test.rb | Corrects test intent around login redirect vs. visibility 404 and asserts broker is not queried for invisible issues. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…143) The redundant match-everything stub relied on mocha precedence to be shadowed; the single counting stub raises on the first attempt by construction.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
test/functional/subscription_issues_controller_test.rb:580
- This test exercises geometry conversion behavior that appears to depend on the optional
redmine_gttplugin (other geometry-related tests in this file are guarded withskip 'redmine_gtt not installed' unless Redmine::Plugin.installed?(:redmine_gtt)). Without the guard, the suite can fail in environments whereredmine_gttisn’t installed.
def test_an_unconvertible_geometry_does_not_fail_the_notification
project = Project.find(1)
project.enabled_module_names = project.enabled_module_names | ['gtt']
@template.update!(geometry_string: '"${location}"')
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Seventh PR from the maintainer review: tests only, no production changes.
The review found the failure coverage asymmetric: the sync action has a broker-failure test for every shape, while publish/unpublish server-side failures, the emitter's non-success responses, and the webhook's 422 contract had none. One federation test also asserted a login redirect under a name promising a 404.
What is now pinned (details in the commit message): the emitter's error logging, PATCH-fallback failure, DELETE 404 tolerance and per-connection isolation; the webhook's all-invalid-batch 422 with errors, mixed-batch 200, and geometry degradation; the publish/unpublish failure matrix (broker error, missing Location, network failure, proxied without a token) with the invariant that
subscription_idnever changes on failure; the federation panel's real visibility guard including that the broker is never queried for an invisible issue; preview's unparsable-body 422; and HTML 403s without the permission.Full suite: 355 runs, 1215 assertions, 0 failures.