Add acceptance test guidelines and the first two scenarios written to them - #5788
Open
johnsimons wants to merge 4 commits into
Open
Add acceptance test guidelines and the first two scenarios written to them#5788johnsimons wants to merge 4 commits into
johnsimons wants to merge 4 commits into
Conversation
Fixes a bug in `When_a_critical_error_is_triggered` where a test double was registered but never resolved, causing the test to run at production intervals. Adds a new guide in `docs/writing-acceptance-tests.md` to help developers avoid silent test failures by following best practices for service overrides and assertions.
Introduce `CustomizeHostBuilderBeforeServiceControl` to allow acceptance tests to register services that ServiceControl's internal registration logic reacts to, such as a transport's `IBrokerThroughputQuery`. The existing `CustomizeHostBuilder` hook remains the preferred way to override services after ServiceControl has been registered.
Verify that licensing usage reports correctly aggregate throughput data from both the message broker and internal monitoring. This test ensures endpoints are grouped correctly by their sanitized names and that the reported figures reflect the maximum throughput across all sources. Includes a fake broker query for testing and updates to the RavenDB acceptance test storage configuration to handle the throughput database.
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
2 participants
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.
Two tests in this suite were recently found to be testing nothing: a double registered against its concrete type, never resolved, passing for years. This writes down how to avoid that, and adds two tests in the shape it argues for.
Guidelines
docs/writing-acceptance-tests.md, linked fromdocs/testing.md.The two scenarios
When_creating_a_usage_report_on_a_non_broker_transportand..._on_a_broker_transport, whichbetween them cover all eight
api/licensingroutes. Those had no acceptance coverage in anysuite. Two tests rather than one because LearningTransport registers no
IBrokerThroughputQuery,so each branch needs its own, and the names say which is which.
Worth a closer look
Three changes that are neither docs nor new tests:
ThroughputDatabaseNamedefaults to a fixed nameand the Raven acceptance storage config set only
DatabaseName, so every acceptance test wassharing one throughput database. The EF suites were unaffected. This is the highest risk change
here, since it touches every Raven acceptance test.
When_a_critical_error_is_triggeredwas not overriding the registration its comment claimedit did. The check ran on its 60 second production interval and the test passed anyway, about
a minute slower than intended. Now 6 seconds instead of 65.
CustomizeHostBuilderBeforeServiceControlhook, for the narrow case of registrationsthat
AddServiceControlreacts to. The MultiInstance runner is updated for the new parameter.Plan
docs/acceptance-test-review-plan.mdcarries the review this came from, including the confirmedlist of routes with no acceptance test. 40 of 85 had none; this closes 8, leaving 32.