Skip to content

[refactor] Replace Path.GetTempPath with AbstractTester in tests #285

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.

Sign up for GitHub

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
marwannettour merged 3 commits into from
Apr 10, 2026
Merged

[refactor] Replace Path.GetTempPath with AbstractTester in tests #285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
638d13d
[refactor] Replace Path.GetTempPath with AbstractTester in tests
marwannettour Apr 8, 2026
cadf6ad
[fix] restore file
marwannettour Apr 8, 2026
b5216ad
[fix] Copilot comments & remove all getfrompath from tests
marwannettour Apr 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions ...eSync.Client.IntegrationTests/Services/Communications/Transfers/R2DownloadResume_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using ByteSync.Common.Business.Actions;
using ByteSync.Common.Business.Inventories;
using ByteSync.Common.Business.SharedFiles;
using ByteSync.TestsCommon;
using ByteSync.DependencyInjection;
using ByteSync.Interfaces.Controls.Communications.Http;
using ByteSync.Interfaces.Factories;
Expand All @@ -22,13 +23,14 @@

namespace ByteSync.Client.IntegrationTests.Services.Communications.Transfers;

public class R2DownloadResume_Tests
public class R2DownloadResume_Tests : AbstractTester
{
private ILifetimeScope _clientScope = null!;

[SetUp]
public void SetUp()
{
CreateTestDirectory();
// ReSharper disable once ConditionIsAlwaysTrueOrFalseAccordingToNullableAPIContract
if (ByteSync.Services.ContainerProvider.Container == null)
{
Comment on lines 30 to 36
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

Now that temp files are created under TestDirectory (in the user's profile) rather than the OS temp location, the test-created files/directories are less likely to be cleaned up by the system. This fixture calls CreateTestDirectory() but never deletes TestDirectory in TearDown, so downloaded/uploaded artifacts may accumulate across runs. Consider deleting TestDirectory in [TearDown] once the test completes.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

@marwannettour marwannettour Apr 9, 2026

Choose a reason for hiding this comment

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

Fixed. The existing [TearDown] already disposed _clientScope; added TestDirectory.Delete(true) to also clean up the test directory, preventing upload/download artifacts (including the 1 MB temp file) from accumulating under the user profile across runs.

Expand Down Expand Up @@ -58,6 +60,11 @@ public void SetUp()
public void TearDown()
{
_clientScope.Dispose();

if (TestDirectory?.Exists == true)
{
TestDirectory.Delete(true);
}
}

[Test]
Expand Down Expand Up @@ -99,7 +106,7 @@ public async Task Download_WithTransientFailure_Should_Retry_And_Succeed()
Source = new SharedDataPart
{
ClientInstanceId = shared.ClientInstanceId,
RootPath = Path.GetTempPath(),
RootPath = TestDirectory.FullName,
InventoryPartType = FileSystemTypes.File,
Name = "itests",
InventoryCodeAndId = "itests"
Expand All @@ -113,7 +120,7 @@ public async Task Download_WithTransientFailure_Should_Retry_And_Succeed()
sag.Targets.Add(new SharedDataPart
{
ClientInstanceId = shared.ClientInstanceId,
RootPath = Path.GetTempFileName(),
RootPath = Path.Combine(TestDirectory.FullName, Guid.NewGuid().ToString("N") + ".tmp"),
InventoryPartType = FileSystemTypes.File,
Name = "itests",
InventoryCodeAndId = "itests"
Expand All @@ -123,7 +130,7 @@ public async Task Download_WithTransientFailure_Should_Retry_And_Succeed()

// First upload a file so we can download it
var inputContent = new string('z', 1_000_000);
var tempFile = Path.GetTempFileName();
var tempFile = Path.Combine(TestDirectory.FullName, Guid.NewGuid().ToString("N") + ".tmp");
await File.WriteAllTextAsync(tempFile, inputContent);

var uploader = uploaderFactory.Build(tempFile, shared);
Expand Down
10 changes: 6 additions & 4 deletions ...eSync.Client.IntegrationTests/Services/Communications/Transfers/R2UploadDownload_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using ByteSync.Client.IntegrationTests.TestHelpers;
using ByteSync.Common.Business.Inventories;
using ByteSync.Common.Business.SharedFiles;
using ByteSync.TestsCommon;
using ByteSync.DependencyInjection;
using ByteSync.Interfaces.Controls.Communications.Http;
using ByteSync.Interfaces.Factories;
Expand All @@ -19,13 +20,14 @@

namespace ByteSync.Client.IntegrationTests.Services.Communications.Transfers;

public class R2UploadDownload_Tests
public class R2UploadDownload_Tests : AbstractTester
{
private ILifetimeScope _clientScope = null!;

[SetUp]
public void SetUp()
{
CreateTestDirectory();
// ReSharper disable once ConditionIsAlwaysTrueOrFalseAccordingToNullableAPIContract
Comment on lines 27 to 31
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

CreateTestDirectory() creates a directory under the user profile, but [TearDown] only disposes _clientScope. Since this test writes temp files under TestDirectory, add deletion of TestDirectory (with an Exists check) in [TearDown] to prevent leftover artifacts across runs.

Copilot uses AI. Check for mistakes.
if (ByteSync.Services.ContainerProvider.Container == null)
{
Comment on lines 27 to 33
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

Now that temp files are created under TestDirectory (in the user's profile) rather than the OS temp location, the test-created files/directories are less likely to be cleaned up by the system. This fixture calls CreateTestDirectory() but never deletes TestDirectory in TearDown, so large artifacts (e.g., the 1MB temp file and any download outputs) will accumulate across runs. Consider deleting TestDirectory in [TearDown] once the test completes.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

@marwannettour marwannettour Apr 9, 2026

Choose a reason for hiding this comment

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

After reviewing the file, TestDirectory is created but never actually written to .It is only used as a string value passed to a mocked dialog service (ShowOpenFolderDialogAsync). No files are ever created inside it, so the directory remains empty after each test. The accumulation impact is negligible (empty directories only), so no teardown cleanup was added here.

Expand Down Expand Up @@ -93,7 +95,7 @@ public async Task Upload_Then_Download_Should_Succeed_With_Small_Chunks()
Source = new SharedDataPart
{
ClientInstanceId = shared.ClientInstanceId,
RootPath = Path.GetTempPath(),
RootPath = TestDirectory.FullName,
InventoryPartType = FileSystemTypes.File,
Name = "itests",
InventoryCodeAndId = "itests"
Expand All @@ -107,7 +109,7 @@ public async Task Upload_Then_Download_Should_Succeed_With_Small_Chunks()
sag.Targets.Add(new SharedDataPart
{
ClientInstanceId = shared.ClientInstanceId,
RootPath = Path.GetTempFileName(),
RootPath = Path.Combine(TestDirectory.FullName, Guid.NewGuid().ToString("N") + ".tmp"),
InventoryPartType = FileSystemTypes.File,
Name = "itests",
InventoryCodeAndId = "itests"
Expand All @@ -116,7 +118,7 @@ public async Task Upload_Then_Download_Should_Succeed_With_Small_Chunks()
sharedActionsGroupRepository.SetSharedActionsGroups([sag]);

var inputContent = new string('x', 1_000_000);
var tempFile = Path.GetTempFileName();
var tempFile = Path.Combine(TestDirectory.FullName, Guid.NewGuid().ToString("N") + ".tmp");
await File.WriteAllTextAsync(tempFile, inputContent);

var uploader = uploaderFactory.Build(tempFile, shared);
Expand Down
12 changes: 7 additions & 5 deletions ...ts/Services/Communications/Transfers/Downloading/SynchronizationDownloadFinalizerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,12 @@
using Microsoft.Extensions.Logging;
using Moq;
using NUnit.Framework;
using ByteSync.TestsCommon;

namespace ByteSync.Client.UnitTests.Services.Communications.Transfers.Downloading;

[TestFixture]
public class SynchronizationDownloadFinalizerTests
public class SynchronizationDownloadFinalizerTests : AbstractTester
{
private Mock<IDeltaManager> _deltaManager = null!;
private Mock<ITemporaryFileManagerFactory> _temporaryFileManagerFactory = null!;
Expand All @@ -24,6 +25,7 @@ public class SynchronizationDownloadFinalizerTests
[SetUp]
public void Setup()
{
CreateTestDirectory();
_deltaManager = new Mock<IDeltaManager>(MockBehavior.Strict);
Comment on lines 25 to 29
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

This fixture creates a new TestDirectory in [SetUp] for every test, but there is no corresponding deletion. AbstractTester does not implement teardown cleanup, so these per-test directories (and any temp files like zips) will accumulate. Consider extending the existing [TearDown] to delete TestDirectory (guarded by Exists).

Copilot uses AI. Check for mistakes.
_temporaryFileManagerFactory = new Mock<ITemporaryFileManagerFactory>(MockBehavior.Strict);
_fileDatesSetter = new Mock<IFileDatesSetter>(MockBehavior.Strict);
Expand Down Expand Up @@ -276,15 +278,15 @@ private static void CreateEntryWithContent(ZipArchive archive, string entryName,
writer.Write(content);
}

private static string GetTempFilePath()
private string GetTempFilePath()
{
var path = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"));
var path = Path.Combine(TestDirectory.FullName, Guid.NewGuid().ToString("N"));

return path;
}

private static string GetNewTempPath(string extension)
private string GetNewTempPath(string extension)
{
return Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N") + extension);
return Path.Combine(TestDirectory.FullName, Guid.NewGuid().ToString("N") + extension);
}
}
11 changes: 6 additions & 5 deletions ...teSync.Client.UnitTests/Services/Comparisons/InventoryComparerIncompletePartsFlatTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,28 +8,29 @@
using ByteSync.Models.Inventories;
using ByteSync.Services.Comparisons;
using FluentAssertions;
using ByteSync.TestsCommon;
using NUnit.Framework;

namespace ByteSync.Client.UnitTests.Services.Comparisons;

[TestFixture]
public class InventoryComparerIncompletePartsFlatTests
public class InventoryComparerIncompletePartsFlatTests : AbstractTester
{
private string _tempDirectory = null!;

[SetUp]
public void Setup()
{
_tempDirectory = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
Directory.CreateDirectory(_tempDirectory);
CreateTestDirectory();
_tempDirectory = TestDirectory.FullName;
}

[TearDown]
public void TearDown()
{
if (Directory.Exists(_tempDirectory))
if (TestDirectory?.Exists == true)
{
Directory.Delete(_tempDirectory, true);
TestDirectory.Delete(true);
}
}

Expand Down
14 changes: 8 additions & 6 deletions ...Sync.Client.UnitTests/Services/Comparisons/InventoryComparerPropagateAccessIssuesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,32 +7,34 @@
using ByteSync.Models.FileSystems;
using ByteSync.Models.Inventories;
using ByteSync.Services.Comparisons;
using ByteSync.TestsCommon;
using FluentAssertions;
using NUnit.Framework;

namespace ByteSync.Client.UnitTests.Services.Comparisons;

[TestFixture]
public class InventoryComparerPropagateAccessIssuesTests
public class InventoryComparerPropagateAccessIssuesTests : AbstractTester
{
private string _tempDirectory = null!;

[SetUp]
public void Setup()
{
_tempDirectory = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
Directory.CreateDirectory(_tempDirectory);
CreateTestDirectory();
_tempDirectory = TestDirectory.FullName;
}

[TearDown]
public void TearDown()
{
if (Directory.Exists(_tempDirectory))
if (TestDirectory?.Exists == true)
{
Directory.Delete(_tempDirectory, true);
TestDirectory.Delete(true);
}
}


Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

The previous implementation created and deleted a unique temp folder per test via [TearDown]. After switching to AbstractTester you no longer delete the created TestDirectory, and AbstractTester itself does not implement a [TearDown] cleanup. This will leave per-test directories under the user's profile and can accumulate over time (especially in CI). Consider reintroducing a [TearDown] here to delete TestDirectory (or otherwise ensure cleanup).

Suggested change
[TearDown]
public void TearDown()
{
if (!string.IsNullOrWhiteSpace(_tempDirectory) && Directory.Exists(_tempDirectory))
{
Directory.Delete(_tempDirectory, true);
}
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

@marwannettour marwannettour Apr 9, 2026

Choose a reason for hiding this comment

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

Fixed. Added a [TearDown] method that deletes TestDirectory after each test, preventing zip inventory files from accumulating under the user profile across runs.

private static string CreateInventoryZipFile(string directory, Inventory inventory)
{
var zipPath = Path.Combine(directory, $"{Guid.NewGuid()}.zip");
Expand Down
12 changes: 7 additions & 5 deletions tests/ByteSync.Client.UnitTests/Services/Configurations/LocalApplicationDataManagerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,19 @@
using FluentAssertions;
using Moq;
using NUnit.Framework;
using ByteSync.TestsCommon;

namespace ByteSync.Client.UnitTests.Services.Configurations;

[TestFixture]
public class LocalApplicationDataManagerTests
public class LocalApplicationDataManagerTests : AbstractTester
{
Comment on lines 15 to 17
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

The PR description says inheriting from AbstractTester "ensures automatic cleanup", but AbstractTester.CreateTestDirectory() only creates a directory and provides no teardown cleanup. Either update the PR description, or ensure each fixture deletes TestDirectory in [TearDown] consistently.

Copilot uses AI. Check for mistakes.
private Mock<IEnvironmentService> _environmentServiceMock = null!;

[SetUp]
public void SetUp()
{
CreateTestDirectory();
_environmentServiceMock = new Mock<IEnvironmentService>();
Comment on lines 20 to 24
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

CreateTestDirectory() allocates a directory under %USERPROFILE%/ByteSync_Automated_Tests/..., but this fixture does not delete TestDirectory after tests that don’t remove tempRoot manually. Because AbstractTester does not auto-cleanup, add a fixture-level [TearDown] that deletes TestDirectory to avoid leaving test artifacts behind.

Copilot uses AI. Check for mistakes.
_environmentServiceMock.SetupGet(e => e.ExecutionMode).Returns(ExecutionMode.Regular);
_environmentServiceMock.SetupProperty(e => e.Arguments, []);
Expand Down Expand Up @@ -81,7 +83,7 @@
public void ApplicationDataPath_Should_Append_CustomSuffix_When_DebugArgumentProvided(OSPlatforms osPlatform)
{
// Arrange
var tempRoot = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"));
var tempRoot = TestDirectory.FullName;
Directory.CreateDirectory(tempRoot);
var assemblyDirectory = Directory.CreateDirectory(Path.Combine(tempRoot, "Portable")).FullName;
var assemblyPath = Path.Combine(assemblyDirectory, "ByteSync.exe");
Expand Down Expand Up @@ -131,7 +133,7 @@
public void ApplicationDataPath_Should_Create_DebugDirectory_When_DebugModeWithoutOverride(OSPlatforms osPlatform)
{
// Arrange
var tempRoot = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"));
var tempRoot = TestDirectory.FullName;
Directory.CreateDirectory(tempRoot);
var assemblyDirectory = Directory.CreateDirectory(Path.Combine(tempRoot, "Portable")).FullName;
var assemblyPath = Path.Combine(assemblyDirectory, "ByteSync.exe");
Expand Down Expand Up @@ -180,7 +182,7 @@
public void LogFilePath_Should_Return_MostRecent_Log_Excluding_Debug()
{
// Arrange
var tempRoot = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"));
var tempRoot = TestDirectory.FullName;
Directory.CreateDirectory(tempRoot);
var assemblyDirectory = Directory.CreateDirectory(Path.Combine(tempRoot, "Portable")).FullName;
var assemblyPath = Path.Combine(assemblyDirectory, "ByteSync.exe");
Expand Down Expand Up @@ -243,7 +245,7 @@
public void DebugLogFilePath_Should_Return_MostRecent_Debug_Log()
{
// Arrange
var tempRoot = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"));
var tempRoot = TestDirectory.FullName;
Directory.CreateDirectory(tempRoot);
var assemblyDirectory = Directory.CreateDirectory(Path.Combine(tempRoot, "Portable")).FullName;
var assemblyPath = Path.Combine(assemblyDirectory, "ByteSync.exe");
Expand Down Expand Up @@ -302,7 +304,7 @@
}
}

private static IDisposable OverrideCommandLineArgs(string[] args)

Check warning on line 307 in tests/ByteSync.Client.UnitTests/Services/Configurations/LocalApplicationDataManagerTests.cs

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Change return type of method 'OverrideCommandLineArgs' from 'System.IDisposable' to 'ByteSync.Client.UnitTests.Services.Configurations.LocalApplicationDataManagerTests.DelegateDisposable' for improved performance 


See more on https://sonarcloud.io/project/issues?id=POW-Software_ByteSync&issues=AZ1s_GYp1MRvjmKtRFUa&open=AZ1s_GYp1MRvjmKtRFUa&pullRequest=285
{
var field = typeof(Environment).GetField("s_commandLineArgs", BindingFlags.Static | BindingFlags.NonPublic);
if (field == null)
Expand Down
Loading
Loading