Modernize: .NET 10, GitHub Actions CI/CD, tests, packaging#1
Modernize: .NET 10, GitHub Actions CI/CD, tests, packaging#1evilz merged 1 commit intoevilz:masterfrom
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
Summary of ChangesHello @evilz-agent, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly modernizes the todo-cli project by upgrading its core framework to .NET 10, revamping the continuous integration and deployment pipeline with GitHub Actions for multi-platform support, and introducing comprehensive unit tests for key functionalities. These changes aim to improve the project's maintainability, streamline the development workflow, and enhance the user experience through easier installation and clearer documentation. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great step in modernizing the project, with upgrades to .NET 10, a new GitHub Actions CI/CD pipeline, and the addition of unit tests. The changes are comprehensive and well-structured.
My review focuses on a few areas for improvement:
- Unit Tests: The newly added tests have some fundamental issues where they test the wrong command structure, leading to incorrect assertions. There's also an opportunity to reduce code duplication in the test setup.
- README: The documentation is much improved, but there are a couple of minor inaccuracies regarding a package source and a link to an external SDK.
- Project Configuration: A minor cleanup in the
.csprojfile can remove a redundant configuration property.
Overall, these are solid changes. Addressing the feedback on the unit tests is critical to ensure they provide real value and correctness guarantees.
| [Fact] | ||
| public void AddCommand_ShouldAcceptTitleArgument() | ||
| { | ||
| // Arrange | ||
| var command = new AddCommand(_serviceProvider); | ||
|
|
||
| // Act | ||
| var titleArgument = command.Children.OfType<Argument>().FirstOrDefault(a => a.Name == "title"); | ||
|
|
||
| // Assert | ||
| Assert.NotNull(titleArgument); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void AddCommand_ShouldAcceptFlags() | ||
| { | ||
| // Arrange | ||
| var command = new AddCommand(_serviceProvider); | ||
|
|
||
| // Act | ||
| var importanceOption = command.Children.OfType<Option>().FirstOrDefault(o => o.Name == "importance"); | ||
| var listOption = command.Children.OfType<Option>().FirstOrDefault(o => o.Name == "list"); | ||
|
|
||
| // Assert | ||
| Assert.NotNull(importanceOption); | ||
| Assert.NotNull(listOption); | ||
| } |
There was a problem hiding this comment.
The tests AddCommand_ShouldAcceptTitleArgument and AddCommand_ShouldAcceptFlags are incorrect. They attempt to find an argument and options on AddCommand itself, but these are actually defined on its item subcommand. Furthermore, the names are incorrect: the argument is subject (not title), and one of the options is star (not importance).
These tests should be rewritten to correctly inspect the item subcommand. This ensures the command structure is verified as intended.
[Fact]
public void AddItemCommand_ShouldAcceptSubjectArgument()
{
// Arrange
var command = new AddCommand(_serviceProvider);
var itemSubCommand = command.Subcommands.Single(c => c.Name == "item");
// Act
var subjectArgument = itemSubCommand.Arguments.SingleOrDefault(a => a.Name == "subject");
// Assert
Assert.NotNull(subjectArgument);
}
[Fact]
public void AddItemCommand_ShouldAcceptListAndStarOptions()
{
// Arrange
var command = new AddCommand(_serviceProvider);
var itemSubCommand = command.Subcommands.Single(c => c.Name == "item");
// Act
var listOption = itemSubCommand.Options.SingleOrDefault(o => o.Name == "list");
var starOption = itemSubCommand.Options.SingleOrDefault(o => o.Name == "star");
// Assert
Assert.NotNull(listOption);
Assert.NotNull(starOption);
}| [Fact] | ||
| public void RemoveCommand_ShouldRequireIdArgument() | ||
| { | ||
| // Arrange | ||
| var command = new RemoveCommand(_serviceProvider); | ||
|
|
||
| // Act | ||
| var idArgument = command.Children.OfType<Argument>().FirstOrDefault(a => a.Name == "id"); | ||
|
|
||
| // Assert | ||
| Assert.NotNull(idArgument); | ||
| } |
There was a problem hiding this comment.
The RemoveCommand_ShouldRequireIdArgument test is incorrect. It checks for an id argument on RemoveCommand, but this argument is actually part of the item subcommand. To correctly verify the command structure, the test should inspect the subcommand.
[Fact]
public void RemoveItemCommand_ShouldRequireIdArgument()
{
// Arrange
var command = new RemoveCommand(_serviceProvider);
var itemSubCommand = command.Subcommands.Single(c => c.Name == "item");
// Act
var idArgument = itemSubCommand.Arguments.SingleOrDefault(a => a.Name == "id");
// Assert
Assert.NotNull(idArgument);
}| # Build the project | ||
| dotnet build | ||
| ```bash | ||
| dotnet tool install -g Todo.CLI --source https://www.nuget.org |
There was a problem hiding this comment.
The dotnet tool install command assumes the package is available on nuget.org. If the package is not yet published or is hosted on a different feed (like GitHub Packages), this command will fail. It would be helpful to either ensure the package is published to NuGet.org or update the instructions to point to the correct package source if it's hosted elsewhere. For example, if using GitHub Packages, you would need to add a --add-source argument.
| - [.NET 10](https://github.com/dotnet/core) | ||
| - [System.CommandLine](https://github.com/dotnet/command-line-api) | ||
| - [Microsoft Graph Beta SDK](https://github.com/microsoftgraph/msgraph-beta-sdk-dotnet) | ||
| - [Microsoft Graph SDK](https://github.com/microsoftgraph/msgraph-beta-sdk-dotnet) |
There was a problem hiding this comment.
The link for "Microsoft Graph SDK" points to the beta SDK repository. Since the project has been updated to use the non-beta Microsoft.Graph v6 package, it would be more accurate to link to the repository for the stable SDK.
| - [Microsoft Graph SDK](https://github.com/microsoftgraph/msgraph-beta-sdk-dotnet) | |
| - [Microsoft Graph SDK](https://github.com/microsoftgraph/msgraph-sdk-dotnet) |
| public class AddCommandTests | ||
| { | ||
| private readonly ServiceProvider _serviceProvider; | ||
|
|
||
| public AddCommandTests() | ||
| { | ||
| _serviceProvider = new ServiceCollection() | ||
| .AddSingleton<TodoCliConfiguration>() | ||
| .AddTodoRepositories() | ||
| .AddSingleton<IUserInteraction>(new MockUserInteraction()) | ||
| .BuildServiceProvider(); | ||
| } |
There was a problem hiding this comment.
The setup logic for _serviceProvider is duplicated in AddCommandTests, ListCommandTests, CompleteCommandTests, and RemoveCommandTests. To improve maintainability and reduce code duplication, consider extracting this common setup into a base class.
For example:
public abstract class CommandTestsBase
{
protected readonly ServiceProvider _serviceProvider;
protected CommandTestsBase()
{
_serviceProvider = new ServiceCollection()
.AddSingleton<TodoCliConfiguration>()
.AddTodoRepositories()
.AddSingleton<IUserInteraction>(new MockUserInteraction())
.BuildServiceProvider();
}
}
public class AddCommandTests : CommandTestsBase
{
// The constructor can be removed if it does nothing else.
// Tests can now use the inherited _serviceProvider field.
[Fact]
public void AddCommand_ShouldHaveCorrectName()
{
// Arrange & Act
var command = new AddCommand(_serviceProvider);
// Assert
Assert.Equal("add", command.Name);
}
// ...
}This would make the test code cleaner and easier to manage.
| <IncludeNativeLibrariesForSelfExtract>true</IncludeNativeLibrariesForSelfExtract> | ||
| <EnableCompressionInSingleFile>true</EnableCompressionInSingleFile> | ||
| <DebugType>embedded</DebugType> | ||
| <ImplicitUsings>enable</ImplicitUsings> |
There was a problem hiding this comment.
The <ImplicitUsings>enable</ImplicitUsings> property is already set globally on line 17. This declaration within the 'Release' configuration property group is redundant and can be removed for clarity.
Co-authored-by: evilz <2937862+evilz@users.noreply.github.com>
2 participants
Summary
This PR modernizes the todo-cli project with the following changes:
Core Upgrades
CI/CD Improvements
Testing
Developer Experience
Checklist
Testing Performed
Generated with ❤️ by OpenClaw