Skip to content

Conversation

@rpdudley
Copy link
Owner

No description provided.

@rpdudley rpdudley linked an issue Oct 29, 2025 that may be closed by this pull request
@rpdudley rpdudley requested a review from Copilot October 29, 2025 21:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds comprehensive unit testing infrastructure using XUnit and refactors configuration management to support both Kubernetes secrets and user secrets. The changes improve testability by exposing internal methods and removing hardcoded settings from appsettings.json.

  • Added XUnit test project with 100+ unit tests covering services, actions, and background services
  • Refactored configuration management to support both Kubernetes and user secrets environments
  • Changed deletion logic from ExecuteDeleteAsync to RemoveRange for better testability
  • Added manual approval step in deployment workflow

Reviewed Changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated 20 comments.

Show a summary per file
File Description
XUnitTest/*.cs 14 new test files covering all major components (services, actions, background services)
XUnitTest/XUnitTest.csproj Test project configuration with XUnit, Moq, and EF Core InMemory dependencies
XUnitTest/test_times.csv Test performance data (100 runs with execution times)
DatabaseProjectAPI/Program.cs Refactored to support user secrets detection and conditional configuration loading
DatabaseProjectAPI/Actions/AutoDeleteAction.cs Changed from ExecuteDeleteAsync to RemoveRange/SaveChanges for testability
DatabaseProjectAPI/Services/StockQuoteBackgroundService.cs Made FetchAndSaveStockDataAsync public for testing
KubsConnect/KubsClient.cs Added new constructor to support user secrets configuration
KubsConnect/Settings/DBConnectionSettings.cs Renamed property from RyanWilliamDB to MySqlDB
KubsConnect/Settings/StartupConfig.cs Removed commented-out code related to old DbConnectionStringSettings
DatabaseProjectAPI/appsettings.json Removed hardcoded API keys (moved to secrets)
.github/workflows/projectdeploy.yml Added manual approval job between build and deploy
DatabaseProjectAPI.sln Added XUnit test project to solution
Comments suppressed due to low confidence (2)

KubsConnect/KubsClient.cs:60

    public T? GetSecret<T>(string K8SNameSpace, string SecretName)

KubsConnect/KubsClient.cs:93

    public T? DoGetSecret<T>(string K8SNameSpace, string SecretName)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}

/// <summary>
/// Constrictor that will only be used when user secrets is available
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected spelling of 'Constrictor' to 'Constructor'.

Suggested change
/// Constrictor that will only be used when user secrets is available
/// Constructor that will only be used when user secrets is available

Copilot uses AI. Check for mistakes.
Comment on lines +34 to +35
run: echo "Waiting for manual approval. Go to GitHub Actions UI and approve the job."

Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approval job does not implement actual approval logic. A simple echo statement will not pause the workflow for manual approval. Consider using GitHub's environment feature with protection rules or the trstringer/manual-approval action to implement proper manual approval gates.

Suggested change
run: echo "Waiting for manual approval. Go to GitHub Actions UI and approve the job."
uses: trstringer/manual-approval@v1
with:
prompt: "Please approve this deployment to proceed."

Copilot uses AI. Check for mistakes.
steps:
- name: Deploy Kubernetes Set Image
run: |
kubectl set image deployments/databaseprojectapi databaseprojectapi=reg.dqcsapp.com/databaseprojectapi:${{env.TAG_ENV}} --kubeconfig=/etc/kube/config
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TAG_ENV environment variable is set in the build-push-deploy job but is referenced in the deploy job. Environment variables do not persist across different jobs. You need to use job outputs or artifacts to pass the tag value between jobs.

Copilot uses AI. Check for mistakes.
Comment on lines +29 to +36
var oldHistories = await _dbContext.StockHistories
.Where(sh => sh.Timestamp < ninetyDaysAgo)
.ToListAsync(cancellationToken);

public async Task DeleteOldStockHistoryAsync(CancellationToken cancellationToken)
{
var ninetyDaysAgo = DateTime.UtcNow.AddDays(-90);

try
{

int deletedCount = await _dbContext.StockHistories
.Where(sh => sh.Timestamp < ninetyDaysAgo)
.ExecuteDeleteAsync(cancellationToken);

if (deletedCount > 0)
{
_logger.LogInformation("{Count} old stock history records deleted.", deletedCount);
}
else
{
_logger.LogInformation("No stock history records found to delete.");
}
}
catch (OperationCanceledException)
if (oldHistories.Any())
{
_logger.LogInformation("Deletion of old stock history was cancelled.");
throw;
_dbContext.StockHistories.RemoveRange(oldHistories);
await _dbContext.SaveChangesAsync(cancellationToken);
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from ExecuteDeleteAsync to ToListAsync().RemoveRange().SaveChanges() significantly impacts performance. This loads all records into memory before deletion, which is inefficient for large datasets. If this change was made solely for testability, consider using a repository pattern or keeping ExecuteDeleteAsync in production while mocking the DbContext properly in tests.

Copilot uses AI. Check for mistakes.
Comment on lines +62 to +69
var oldLogs = await _dbContext.ApiCallLog
.Where(log => log.CallDate < ninetyDaysAgo)
.ToListAsync(cancellationToken);

if (oldLogs.Any())
{
_logger.LogInformation("Deletion of old API call logs was cancelled.");
throw;
_dbContext.ApiCallLog.RemoveRange(oldLogs);
await _dbContext.SaveChangesAsync(cancellationToken);
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from ExecuteDeleteAsync to ToListAsync().RemoveRange().SaveChanges() significantly impacts performance. This loads all records into memory before deletion, which is inefficient for large datasets. If this change was made solely for testability, consider using a repository pattern or keeping ExecuteDeleteAsync in production while mocking the DbContext properly in tests.

Copilot uses AI. Check for mistakes.
namespace XUnitTests;
public class AutoDeleteActionTests
{
private DpapiDbContext _dbContext;
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Field '_dbContext' can be 'readonly'.

Suggested change
private DpapiDbContext _dbContext;
private readonly DpapiDbContext _dbContext;

Copilot uses AI. Check for mistakes.
public class AutoDeleteActionTests
{
private DpapiDbContext _dbContext;
private AutoDeleteAction _autoDeleteAction;
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Field '_autoDeleteAction' can be 'readonly'.

Suggested change
private AutoDeleteAction _autoDeleteAction;
private readonly AutoDeleteAction _autoDeleteAction;

Copilot uses AI. Check for mistakes.
{
private DpapiDbContext _dbContext;
private AutoDeleteAction _autoDeleteAction;
private Mock<ILogger<AutoDeleteAction>> _loggerMock;
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Field '_loggerMock' can be 'readonly'.

Suggested change
private Mock<ILogger<AutoDeleteAction>> _loggerMock;
private readonly Mock<ILogger<AutoDeleteAction>> _loggerMock;

Copilot uses AI. Check for mistakes.
namespace XUnitTests;
public class TrackedStockActionTests
{
private DpapiDbContext _dbContext;
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Field '_dbContext' can be 'readonly'.

Suggested change
private DpapiDbContext _dbContext;
private readonly DpapiDbContext _dbContext;

Copilot uses AI. Check for mistakes.
public class TrackedStockActionTests
{
private DpapiDbContext _dbContext;
private TrackedStockAction _trackedStockAction;
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Field '_trackedStockAction' can be 'readonly'.

Suggested change
private TrackedStockAction _trackedStockAction;
private readonly TrackedStockAction _trackedStockAction;

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add XUnit Test for the Actions folder

3 participants