Move creation of selected git commands from GitModule
#11308
Conversation
|
Some tests like those in UnitTests/GitCommands.Tests/Git/CommandsTests.cs seem to be removed. Is that intentional? |
|
I didn't delete any tests, just moved those around.
But I'll double check
|
There was a problem hiding this comment.
It just looks like lots of tests had been removed in the first commit. That is because testcases have been moved out to a new file:
--- a/UnitTests/GitCommands.Tests/Git/Commands/GitCommandHelpersTest.cs
+++ b/UnitTests/GitCommands.Tests/Git/Commands/GitCommandHelpersTest.cs
--- a/UnitTests/GitCommands.Tests/Git/Commands/GitCommandHelpersTest.cs
+++ b/UnitTests/GitCommands.Tests/Git/CommandsTests.cs
But what is the criterion for this split of GitCommandHelpersTest.cs?
Move tests to match folder structure
For technical reasons (avoiding ambiguity), the namespace name was tinkered.
namespace GitCommandsTests.Git_Commands
means
namespace GitCommandsTests.Git.Commands
(If you think this is a problem, we should rather switch to namespace UnitTests.GitCommands_.Git_.Commands_ for instance.)
Hence the "git tag" tests should not be moved up out of the folder UnitTests/GitCommands.Tests/Git/Commands/:
--- a/UnitTests/GitCommands.Tests/Git/Commands/GitCreateTagCmdTests.cs
+++ b/UnitTests/GitCommands.Tests/Git/CommandsTests.CreateTag.cs
@@ -4,8 +4,7 @@
namespace GitCommandsTests.Git_Commands
{
- [TestFixture]
- public sealed class GitCreateTagCmdTests
+ partial class CommandsTests
🤔 Test names have lost the relation to the command:
| Before | After |
|---|---|
It was easier to rename GitDeleteBranchCmdTests.cs to CommandsTests.DeleteBranch.cs than to move the content and make the review harder. The same applies to CommandsTests.CreateTag.cs. |
This means the original tests were poorly named, and I didn't pay much attention to those. My goal was to consolidate the tests and place those in the correct folder structure. |
GitCommandHelpersTest contain tests for commands defined in GitModule class. Those have to be moved out (#11308 is contributing to that. However, there are few methods like |
Note: `useExplicitCommitMessage` was changed to `false` by default
3a9e2fc to
719b950
Compare
| { !string.IsNullOrEmpty(author), $"--author=\"{author?.Trim().Trim('"')}\"" }, | ||
| { gpgSign && string.IsNullOrWhiteSpace(gpgKeyId), "-S" }, | ||
| { gpgSign && !string.IsNullOrWhiteSpace(gpgKeyId), $"-S{gpgKeyId}" }, | ||
| { useExplicitCommitMessage, $"-F \"{GetGitExecPath(Path.Combine(GetGitDirectory(), "COMMITMESSAGE"))}\"" }, |
There was a problem hiding this comment.
This change prevents commits in WSL.
Only in master.
Not sure if other commands are affected, refactoring moving code are hard to review.
There was a problem hiding this comment.
This is the only real change. A comment would have been good. How does this make it work in WSL?
There was a problem hiding this comment.
The path is in the format for gitexec, ie in wsl format.
Added for a few commands, manually identified.
This was a series of changes, I did not see any related changes. So hopefully the only issue.
The transformation will have to be added to the caller of commands arguments where module is available.
Proposed changes
Merge strategy
❗ DO NOT SQUASH
✒️ I contribute this code under The Developer Certificate of Origin.