-
Notifications
You must be signed in to change notification settings - Fork 0
fix(skill): correct TestDoubles matching guidance #113
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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
5139507
fix(skill): correct TestDoubles matching guidance
ncipollina caae88c
fix(skill): repair shared double examples
ncipollina e68a5db
fix(skill): correct namespace in shared-double benchmark output
ncipollina 04d5d46
fix(skill): add old-skill baseline for testdoubles benchmark
ncipollina 77ead53
fix(skill): rerun testdoubles benchmark as a true paired batch
ncipollina c86a166
docs(skill): fix self-contradictory baseline wording in benchmark.md
ncipollina File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
113 changes: 113 additions & 0 deletions
113
docs/research/0012-aws-secrets-manager-provider-compono-skill-regression.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| # [RESEARCH-0012] AWS Secrets Manager Provider Compono Skill Regression | ||
|
|
||
| **Status:** Done | ||
|
|
||
| **Date:** 2026-08-27 | ||
|
|
||
| ## Scope | ||
|
|
||
| This records two focused follow-up findings from the real AWS Secrets Manager | ||
| Provider migration from AutoFixture/AutoFixture.Xunit3/NSubstitute to | ||
| `Compono.XunitV3` + `Compono.TestDoubles`. | ||
|
|
||
| This is not a product design record and creates no ADR. It applies the existing | ||
| ADR-0029/ADR-0042 evidence rules to classify what the migration actually showed. | ||
|
|
||
| ## Finding A: installed skill missed shipped argument matching support | ||
|
|
||
| During the migration, the installed Compono skill initially led the agent to | ||
| write hand-made recording fakes for `IAmazonSecretsManager`, | ||
| `IConfigurationBuilder`, and `ILoggerFactory`, asserting that | ||
| `Compono.TestDoubles` intentionally did not support argument matchers/capture. | ||
| That was incorrect for ordinary matching and filtered verification. | ||
|
|
||
| The final migration used the shipped `Compono.TestDoubles` surface directly: | ||
|
|
||
| ```csharp | ||
| configurationBuilder.Configure() | ||
| .Add(Match.Any<IConfigurationSource>()) | ||
| .Returns(configurationBuilder); | ||
|
|
||
| configurationBuilder.Verify() | ||
| .Add(Match.Is<IConfigurationSource>(predicate)) | ||
| .Once(); | ||
| ``` | ||
|
|
||
| and: | ||
|
|
||
| ```csharp | ||
| secretsManager.Configure() | ||
| .GetSecretValueAsync( | ||
| Match.Is<GetSecretValueRequest>(predicate), | ||
| Match.Any<CancellationToken>()) | ||
| .Returns(Task.FromResult(response)); | ||
| ``` | ||
|
|
||
| The migrated suite passed across `net8.0`, `net9.0`, `net10.0`, and `net11.0` | ||
| (62/62 on each TFM), with AutoFixture/NSubstitute/`Compono.NSubstitute` absent | ||
| from direct and transitive dependencies. | ||
|
|
||
| Classification: **skill/docs regression**, not a runtime `Compono.TestDoubles` | ||
| capability gap. Current `Compono.TestDoubles` supports literal equality | ||
| matching, `Match.Any<T>()`, `Match.Is<T>(predicate)`, argument-filtered | ||
| `Never()`/`Once()`/`Exactly(n)`, and multi-entry argument-distinguished response | ||
| configuration for eligible member shapes. True capture/callback behavior remains | ||
| a separate boundary. | ||
|
|
||
| ## Finding B: `TestConfigurationProvider` is existing project-local test architecture | ||
|
|
||
| The migration retained a local `TestConfigurationProvider : ConfigurationProvider`. | ||
| This is not new evidence for abstract-class generated doubles. | ||
|
|
||
| Pre-migration evidence from the AWS Secrets Manager Provider test project: | ||
|
|
||
| ```csharp | ||
| public class ConfigurationProviderSpecimenBuilder : ISpecimenBuilder | ||
| { | ||
| public object Create(object request, ISpecimenContext context) | ||
| { | ||
| if (request is Type type && type == typeof(ConfigurationProvider)) | ||
| { | ||
| return new TestConfigurationProvider(); | ||
| } | ||
|
|
||
| return new NoSpecimen(); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ```csharp | ||
| public class TestConfigurationProvider : ConfigurationProvider | ||
| { | ||
| public override void Set(string key, string value) | ||
| { | ||
| Data[key] = value; | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| The fixture registered that specimen builder before `AutoNSubstituteCustomization`: | ||
|
|
||
| ```csharp | ||
| fixture.Customizations.Add(new ConfigurationProviderSpecimenBuilder()); | ||
|
|
||
| fixture.Customize(new AutoNSubstituteCustomization | ||
| { | ||
| GenerateDelegates = true | ||
| }); | ||
| ``` | ||
|
|
||
| Search of the pre-migration state found no `Substitute.For<ConfigurationProvider>()` | ||
| and no NSubstitute-backed substitution of `ConfigurationProvider`. | ||
|
|
||
| Classification: **existing project-local test architecture**, not a Compono | ||
| capability gap. Compono preserves the same design through: | ||
|
|
||
| ```csharp | ||
| builder.Register<ConfigurationProvider>(_ => new TestConfigurationProvider()); | ||
| ``` | ||
|
|
||
| ADR-0042 Amendment 2 does not apply because there is no evidenced | ||
| `Compono.NSubstitute`/NSubstitute-can vs. `Compono.TestDoubles`-cannot | ||
| replacement case. No roadmap item, ADR, or abstract-class support design is | ||
| created from this evidence. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| { | ||
| "skill_name": "compono", | ||
| "date": "2026-08-27", | ||
| "runner": "pi --print --no-context-files --no-skills --skill <skill-path> --thinking minimal", | ||
| "scope": "Focused AWS Secrets Manager Provider TestDoubles matching/capture regression evals only", | ||
| "baseline": { | ||
| "type": "old_skill", | ||
| "snapshot_commit": "644a5ad", | ||
| "note": "Full skills/compono directory (SKILL.md + references/) snapshotted at 644a5ad, the commit before this PR's fix (5139507, caae88c). All 8 runs (4 evals x with_skill/old_skill) launched together in one batch so with_skill and old_skill are genuinely paired, per skill-creator's workflow." | ||
| }, | ||
| "results": [ | ||
| { | ||
| "eval_id": 28, | ||
| "name": "aws-iconfigurationbuilder-argument-filtered-verification", | ||
| "configuration": "with_skill", | ||
| "passed": true, | ||
| "evidence": "Output uses Configure().Add(Match.Any<IConfigurationSource>()).Returns(configurationBuilder) and Verify().Add(Match.Is<IConfigurationSource>(...)).Once(), maps Arg.Is to Match.Is, and does not recommend a recording fake." | ||
| }, | ||
| { | ||
| "eval_id": 28, | ||
| "name": "aws-iconfigurationbuilder-argument-filtered-verification", | ||
| "configuration": "old_skill", | ||
| "passed": false, | ||
| "evidence": "Output claims Compono.TestDoubles cannot express the argument predicate, uses zero-arg Configure().Add()/Verify().Add().Once(), and says asserting the argument type still requires keeping the test on NSubstitute." | ||
| }, | ||
| { | ||
| "eval_id": 29, | ||
| "name": "aws-iamazonsecretsmanager-argument-matched-configuration", | ||
| "configuration": "with_skill", | ||
| "passed": true, | ||
| "evidence": "Output uses Configure().GetSecretValueAsync(Match.Is<GetSecretValueRequest>(...), Match.Any<CancellationToken>()).Returns(...) and maps Arg.Is/Arg.Any directly to Match.Is/Match.Any." | ||
| }, | ||
| { | ||
| "eval_id": 29, | ||
| "name": "aws-iamazonsecretsmanager-argument-matched-configuration", | ||
| "configuration": "old_skill", | ||
| "passed": false, | ||
| "evidence": "Output drops the argument matcher entirely, uses parameterless Configure().GetSecretValueAsync().Returns(...), and states 'Compono.TestDoubles is argument-independent' explicitly." | ||
| }, | ||
| { | ||
| "eval_id": 30, | ||
| "name": "adversarial-nsubstitute-vocabulary-trap", | ||
| "configuration": "with_skill", | ||
| "passed": true, | ||
| "evidence": "Output maps Arg.Is/Arg.Any/Received/DidNotReceive to Match.Is/Match.Any/Verify().Once/Exactly/Never and rejects recording fakes solely for NSubstitute vocabulary." | ||
| }, | ||
| { | ||
| "eval_id": 30, | ||
| "name": "adversarial-nsubstitute-vocabulary-trap", | ||
| "configuration": "old_skill", | ||
| "passed": false, | ||
| "evidence": "Output says 'Arg.Any/Arg.Is usually disappears because generated doubles are argument-independent' and lists argument-specific returns/verification as behavior Compono.TestDoubles does not provide, contradicting the expected Arg.Is->Match.Is / Arg.Any->Match.Any mapping." | ||
| }, | ||
| { | ||
| "eval_id": 31, | ||
| "name": "true-callback-capture-boundary", | ||
| "configuration": "with_skill", | ||
| "passed": true, | ||
| "evidence": "Output says ordinary matching/filtering is supported but invocation-aware callbacks/capture/delegate invocation are outside Match<T>; recommends local fake or NSubstitute seam." | ||
| }, | ||
| { | ||
| "eval_id": 31, | ||
| "name": "true-callback-capture-boundary", | ||
| "configuration": "old_skill", | ||
| "passed": false, | ||
| "evidence": "Output correctly recommends a local fake for the callback boundary, but does so by claiming 'no argument matchers' / 'no argument-aware behavior' exist at all in Compono.TestDoubles, contradicting the expectation that ordinary matching/argument-filtered verification is supported." | ||
| } | ||
| ], | ||
| "summary": { | ||
| "with_skill": { "passed": 4, "total": 4, "pass_rate": 1.0 }, | ||
| "old_skill": { "passed": 0, "total": 4, "pass_rate": 0.0 }, | ||
| "old_eval_regression_status": "not_rerun_full_benchmark_claude_weekly_limit; existing eval definitions 1-27 unchanged" | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.