Conversation
- Upgrade to .NET 10 (from .NET 8) - Add GitHub Actions workflow (CI/CD) - Add unit tests for commands - Improve self-contained publishing for Linux/Windows/macOS - Update README with install instructions
…into copilot/implement-feedback-from-gemini
Co-authored-by: evilz <2937862+evilz@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request attempts to implement feedback from a gemini-code-assist review on PR #1, focusing on fixing test structure and reducing code duplication. However, it contains critical issues that prevent it from building or running.
Changes:
- Upgrade from .NET 8 to .NET 10 (which doesn't exist yet)
- Fix test structure to properly check subcommands and their arguments/options
- Introduce
CommandTestsBaseabstract class to reduce duplicated service provider setup in tests
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Todo.Core/Todo.Core.csproj | Updates target framework to net10.0 and dependency versions to 10.0.0 |
| src/Todo.CLI/Todo.CLI.csproj | Updates target framework to net10.0, dependency versions to 10.0.0, and adds redundant RuntimeIdentifier properties |
| src/Todo.CLI.Tests/Todo.CLI.Tests.csproj | Updates target framework to net10.0 and test dependency versions |
| src/Todo.CLI.Tests/Commands/CommandTests.cs | Introduces new CommandTestsBase with test classes but has missing imports and constructor parameter issues |
| README.md | Updates documentation to reference .NET 10 and correctly updates Microsoft Graph SDK link |
| .github/workflows/ci.yml | Configures CI/CD for .NET 10.0.x with multi-platform builds |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <RuntimeIdentifier>linux-x64</RuntimeIdentifier> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- Windows x64 Release --> | ||
| <PropertyGroup Condition="'$(Configuration)|$(RuntimeIdentifier)' == 'Release|win-x64'"> | ||
| <SelfContained>true</SelfContained> | ||
| <RuntimeIdentifier>win-x64</RuntimeIdentifier> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- macOS x64 Release --> | ||
| <PropertyGroup Condition="'$(Configuration)|$(RuntimeIdentifier)' == 'Release|osx-x64'"> | ||
| <SelfContained>true</SelfContained> | ||
| <RuntimeIdentifier>osx-x64</RuntimeIdentifier> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- macOS ARM64 Release --> | ||
| <PropertyGroup Condition="'$(Configuration)|$(RuntimeIdentifier)' == 'Release|osx-arm64'"> | ||
| <SelfContained>true</SelfContained> | ||
| <RuntimeIdentifier>osx-arm64</RuntimeIdentifier> |
There was a problem hiding this comment.
Setting RuntimeIdentifier within a conditional PropertyGroup where the condition already includes that RuntimeIdentifier is redundant. The property condition '$(Configuration)|$(RuntimeIdentifier)' == 'Release|linux-x64' already evaluates the RuntimeIdentifier, so setting it again inside is unnecessary and could potentially cause evaluation issues. Consider removing the explicit RuntimeIdentifier setting within these conditional blocks, as it's already defined by the condition itself.
| <RuntimeIdentifier>linux-x64</RuntimeIdentifier> | |
| </PropertyGroup> | |
| <!-- Windows x64 Release --> | |
| <PropertyGroup Condition="'$(Configuration)|$(RuntimeIdentifier)' == 'Release|win-x64'"> | |
| <SelfContained>true</SelfContained> | |
| <RuntimeIdentifier>win-x64</RuntimeIdentifier> | |
| </PropertyGroup> | |
| <!-- macOS x64 Release --> | |
| <PropertyGroup Condition="'$(Configuration)|$(RuntimeIdentifier)' == 'Release|osx-x64'"> | |
| <SelfContained>true</SelfContained> | |
| <RuntimeIdentifier>osx-x64</RuntimeIdentifier> | |
| </PropertyGroup> | |
| <!-- macOS ARM64 Release --> | |
| <PropertyGroup Condition="'$(Configuration)|$(RuntimeIdentifier)' == 'Release|osx-arm64'"> | |
| <SelfContained>true</SelfContained> | |
| <RuntimeIdentifier>osx-arm64</RuntimeIdentifier> | |
| </PropertyGroup> | |
| <!-- Windows x64 Release --> | |
| <PropertyGroup Condition="'$(Configuration)|$(RuntimeIdentifier)' == 'Release|win-x64'"> | |
| <SelfContained>true</SelfContained> | |
| </PropertyGroup> | |
| <!-- macOS x64 Release --> | |
| <PropertyGroup Condition="'$(Configuration)|$(RuntimeIdentifier)' == 'Release|osx-x64'"> | |
| <SelfContained>true</SelfContained> | |
| </PropertyGroup> | |
| <!-- macOS ARM64 Release --> | |
| <PropertyGroup Condition="'$(Configuration)|$(RuntimeIdentifier)' == 'Release|osx-arm64'"> | |
| <SelfContained>true</SelfContained> |
| _serviceProvider = new ServiceCollection() | ||
| .AddSingleton<TodoCliConfiguration>() | ||
| .AddTodoRepositories() | ||
| .AddSingleton<IUserInteraction>(new MockUserInteraction()) |
There was a problem hiding this comment.
MockUserInteraction is instantiated without the required constructor parameters. According to the constructor in MockUserInteraction.cs, it requires an IEnumerable parameter and optionally a bool parameter. This code will not compile. Consider passing appropriate default values, such as: new MockUserInteraction(Array.Empty())
| using System.CommandLine; | ||
| using Todo.CLI.Commands; | ||
| using Todo.CLI.Tests.Handlers; | ||
| using Todo.Core; | ||
| using Xunit; |
There was a problem hiding this comment.
Missing using directive for Microsoft.Extensions.DependencyInjection namespace. The code uses ServiceCollection and ServiceProvider types which require this import. Other test files in the same directory (AddCommandTests.cs, CompleteCommandTests.cs, ListCommandTests.cs, RemoveCommandTests.cs) all explicitly import this namespace. Add: using Microsoft.Extensions.DependencyInjection;
Implements all feedback from gemini-code-assist review on PR #1.
Critical Test Fixes
Tests were inspecting the wrong command level.
AddCommandandRemoveCommanduse subcommands (item/list), but tests checked the parent command:Fixed argument/option names:
title→subjectimportance→starCode Quality
CommandTestsBaseabstract class to eliminate duplicated service provider initialization across 4 test classesImplicitUsingsin Release configuration (already set globally)Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.