test: cover file handling as shipped, and fix the .runsettings gitignore path - #659
Merged
Merged
Conversation
…ore path Adds unit and integration coverage for the inbound file handling shipped in PR A, and fixes a .gitignore rule that let integration test secrets be committed. The gitignore rule covered only the pre-migration core/test/IntegrationTests/.runsettings/ path, while test/IntegrationTests/README.md tells contributors to place secrets in test/IntegrationTests/.runsettings/ and describes it as ignored. Secrets written to the documented location were tracked. File handling previously had no presence in the integration suite. FilesIntegrationTests covers DI resolution, a real streamed fetch, a live 401 mapping onto FileUrlExpiredException, and the scope and https gates. FilesAccessorWireShapeTests maps the shape Teams actually sends, which no existing test used. FileDownloaderTransportTests asserts no Authorization header reaches the storage host, which teams.ts already covered and C# did not. No product changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf9d8c98-4629-4ddc-8b36-2c6a82057f94
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is test-focused and aims to (1) prevent accidental secret check-in by fixing the .runsettings gitignore coverage for the documented integration-test location, and (2) add/extend tests that characterize inbound file handling and downloader transport invariants “as shipped”.
Changes:
- Add unit tests that validate file-attachment mapping against a minimal “real wire” JSON shape and ensure non-file sibling attachments are ignored.
- Add transport-focused unit tests to assert the file download client does not send
Authorizationheaders to third-party storage hosts. - Add new integration tests for
FileDownloaderDI resolution and live-network contract behavior; cap member-logging loops; update.gitignoreto ignore the documented.runsettingsdirectory.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.Teams.Apps.UnitTests/Files/FilesAccessorWireShapeTests.cs | New unit tests validating attachment mapping against a minimal wire-shape JSON fixture (two attachments, missing etag, missing text). |
| test/Microsoft.Teams.Apps.UnitTests/Files/FileDownloaderTransportTests.cs | New unit tests asserting the downloader’s DI registration sends no Authorization and uses a plain GET request shape. |
| test/IntegrationTests/FilesIntegrationTests.cs | New integration tests validating FileDownloader resolves from the app container and matches a live-network byte-stream/401 contract and scope/https guards. |
| test/IntegrationTests/ConversationClientTests.cs | Cap member logging to the first 5 members to reduce log volume. |
| test/IntegrationTests/CompatTeamsInfoTests.cs | Cap member logging to the first 5 members across multiple member-listing tests to reduce log volume. |
| .gitignore | Ignore test/IntegrationTests/.runsettings/ to prevent secrets in the documented location from being tracked. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…e-registered one BuildAppContainer called AddHttpClient<FileDownloader>() itself, which registered the downloader independently of the hosting extensions. The transport tests therefore kept passing even when AddTeamsBotApplication stopped registering FileDownloader at all, which defeats the purpose of tests whose stated job is guarding that registration. Override the primary handler on the existing typed client's HttpClientFactoryOptions instead, so resolution fails outright if the production registration goes away. Verified by mutation: removing the registration from the hosting extensions now fails these tests with "No service for type FileDownloader has been registered", where previously they passed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf9d8c98-4629-4ddc-8b36-2c6a82057f94
… writing secrets The README described the .runsettings directory as gitignored, which was the claim that made the gap invisible: the ignore rule tracked only the pre-migration core/test path, so secrets written to the documented location were tracked. Point readers at git check-ignore rather than asking them to trust the sentence, since older branches still carry the stale rule. Also warn against diagnostic verbosity on this suite, which can echo environment variables while these files hold a live client secret. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf9d8c98-4629-4ddc-8b36-2c6a82057f94
The claim that diagnostic verbosity echoes environment variables was asserted rather than measured, and it does not reproduce: a `-v d` run of this suite emitted the runsettings client secret zero times and no `AzureAd__*` names, because VSTest applies those values to the test host process rather than the MSBuild command line. Keep the gitignore verification step, which was verified with git check-ignore. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf9d8c98-4629-4ddc-8b36-2c6a82057f94
Mehak Bindra (MehakBindra)
approved these changes
Aug 26, 2026
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.
Test-only.
src/untouched.Tests
FilesIntegrationTests.cs(5, new). File handling had no presence in the integration suite. Covers DI resolution (catches removal ofAddHttpClient<FileDownloader>(), which otherwise falls back to a static client silently and with no log), a real streamed fetch, a live 401 mapping toFileUrlExpiredException, and the scope/https gates.FilesAccessorWireShapeTests.cs(3). Mapping against the shape Teams actually sends, which no existing test uses: two attachments, noetag, notext. Fixture is allowlist-built from the seven fields the mapper reads, as raw wire JSON so absent fields stay absent.FileDownloaderTransportTests.cs(3). Asserts noAuthorizationreaches the storage host. teams.ts has this; C# didn't. The guarantee lives in the registration, not the downloader, so these assert against the real container.Also caps three member-logging loops with
.Take(5), finishing a migration already applied elsewhere. Uncapped they dump the tenant roster into CI logs.Scope
High fidelity for the download contract, low for provenance. These aren't Teams
tempauthURLs, so this is not end-to-end file receive. Graph is used only as a stable unauthenticated endpoint; no token, no Graph permissions.Validation
767 unit tests pass. Integration: 77 total, 60 passed, 12 failed, 5 skipped. Format clean; builds net8.0 and net10.0.
Each test verified by mutation: deleting the DI registration fails all 5 integration tests; attaching auth to the registration fails exactly the one transport test guarding it.