Skip to content

test: remove redundant tests #34

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
loadinglucian merged 4 commits into from
Oct 10, 2025
Merged

test: remove redundant tests #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c626fa7
test(repository): remove redundant server loading test
loadinglucian Oct 10, 2025
d51f5ad
test(server-add): consolidate redundant tests
loadinglucian Oct 10, 2025
7c6428a
test(server-delete): consolidate display tests with datasets
loadinglucian Oct 10, 2025
27dbafb
test(server-list): remove redundant field display test
loadinglucian Oct 10, 2025
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
100 changes: 13 additions & 87 deletions tests/Integration/Console/Server/ServerAddCommandTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,116 +253,42 @@ function createServerAddCommandTester(?SSHService $sshService = null): CommandTe
->and($output)->toContain('--skip');
});

it('saves server when confirmation is given', function () {
// ARRANGE
$sshService = mockSSHServiceWithBehavior(true);
$tester = createServerAddCommandTester($sshService);

// ACT - Provide all required options
ob_start();
$exitCode = $tester->execute([
'--name' => 'confirmed-server',
'--host' => '192.168.1.1',
'--port' => '22',
'--username' => 'root',
'--private-key-path' => '',
'--skip' => true,
'--yes' => true,
]);
ob_end_clean();

// ASSERT
$output = $tester->getDisplay();
expect($exitCode)->toBe(Command::SUCCESS)
->and($output)->toContain('✓')
->and($output)->toContain('Server added successfully');
});

//
// Inventory Persistence
// Inventory Persistence & Display
// -------------------------------------------------------------------------------

it('persists server data to inventory correctly', function () {
// ARRANGE
$sshService = mockSSHServiceWithBehavior(true);
$tester = createServerAddCommandTester($sshService);

// ACT - Provide all required options
ob_start();
$tester->execute([
'--name' => 'persisted-server',
'--host' => '10.20.30.40',
'--port' => '8022',
'--username' => 'admin',
'--private-key-path' => '~/.ssh/admin_key',
'--skip' => true,
'--yes' => true,
]);
ob_end_clean();

// ASSERT - Verify server persisted by checking command output
$output = $tester->getDisplay();
expect($output)->toContain('✓')
->and($output)->toContain('Server added successfully')
->and($output)->toContain('persisted-server')
->and($output)->toContain('10.20.30.40');
});

it('displays complete server information before saving', function () {
it('displays complete server information and persists to inventory', function () {
// ARRANGE
$sshService = mockSSHServiceWithBehavior(true);
$tester = createServerAddCommandTester($sshService);

// ACT - Provide all required options
ob_start();
$tester->execute([
'--name' => 'display-test',
$exitCode = $tester->execute([
'--name' => 'complete-test',
'--host' => 'example.com',
'--port' => '22',
'--port' => '8022',
'--username' => 'deployer',
'--private-key-path' => '~/.ssh/key',
'--skip' => true,
'--yes' => true,
]);
ob_end_clean();

// ASSERT
// ASSERT - Verify display AND persistence
$output = $tester->getDisplay();
expect($output)->toContain('Name:')
->and($output)->toContain('display-test')
expect($exitCode)->toBe(Command::SUCCESS)
->and($output)->toContain('Name:')
->and($output)->toContain('complete-test')
->and($output)->toContain('Host:')
->and($output)->toContain('example.com')
->and($output)->toContain('Port:')
->and($output)->toContain('22')
->and($output)->toContain('8022')
->and($output)->toContain('User:')
->and($output)->toContain('deployer')
->and($output)->toContain('Key:')
->and($output)->toContain('~/.ssh/key');
});

it('shows default SSH key path when not provided', function () {
// ARRANGE
$sshService = mockSSHServiceWithBehavior(true);
$tester = createServerAddCommandTester($sshService);

// ACT - Provide all required options except private-key-path to test default
ob_start();
$tester->execute([
'--name' => 'default-key',
'--host' => '192.168.1.1',
'--port' => '22',
'--username' => 'root',
'--private-key-path' => '',
'--skip' => true,
'--yes' => true,
]);
ob_end_clean();

// ASSERT
$output = $tester->getDisplay();
expect($output)->toContain('Key:')
->and($output)->toContain('default')
->and($output)->toContain('~/.ssh/id_ed25519')
->and($output)->toContain('~/.ssh/id_rsa');
->and($output)->toContain('~/.ssh/key')
->and($output)->toContain('✓')
->and($output)->toContain('Server added successfully');
});
Comment on lines +260 to 293

@coderabbitai coderabbitai Bot Oct 10, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Clarify indirect persistence verification in comment.

The test verifies persistence indirectly through the success exit code and message, which is appropriate for integration tests. However, the comment at line 278 says "Verify display AND persistence" without noting this is indirect verification.

Consider updating the comment to be more explicit:

-        // ASSERT - Verify display AND persistence
+        // ASSERT - Verify display and indirect persistence (via success indicators)

Also, note that the test uses a custom key path. Explicit verification of default key path display (when --private-key-path is empty) appears to have been removed, though it's still implicitly tested by the "adds server with minimal options using defaults" test at lines 61-87.

Run the following verification to confirm default key path display is still tested:

🏁 Script executed:

#!/bin/bash
# Description: Verify that default SSH key path display is tested

# Search for test assertions checking default key path output
rg -nP --type=php -A5 -B5 'id_ed25519|id_rsa' tests/Integration/Console/Server/ServerAddCommandTest.php

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Search for any default SSH key path references in the test
rg -n "~/.ssh" -C3 tests/Integration/Console/Server/ServerAddCommandTest.php

Length of output: 867

🏁 Script executed:

#!/bin/bash
# Locate minimal-options test to verify default key-path assertion
rg -n --type=php "minimal options" -C5 tests/Integration/Console/Server/ServerAddCommandTest.php

Length of output: 479

Clarify indirect persistence verification.

  • Update the assertion comment at line 278:
-        // ASSERT - Verify display AND persistence
+        // ASSERT - Verify display and indirect persistence (via exit code + message)
  • Optional: In the “adds server with minimal options using defaults” test (lines 61–87), add an assertion for the default SSH key path (e.g., ~/.ssh/id_rsa).
🤖 Prompt for AI Agents 
In tests/Integration/Console/Server/ServerAddCommandTest.php around lines 260 to
293 (specifically update the assertion comment at line 278), clarify that the
test verifies display output and indirectly confirms persistence to inventory by
checking the success message and output strings; replace the vague comment "//
ASSERT - Verify display AND persistence" with a clearer one like "// ASSERT -
Verify display output and that command indicates server was persisted (success
message used as indirect persistence check)". Also optionally add an assertion
in the "adds server with minimal options using defaults" test (lines 61–87) to
assert the default SSH key path (e.g., expect output or stored value toContain
'~/.ssh/id_rsa') so the default key behavior is explicitly tested.

});
73 changes: 16 additions & 57 deletions tests/Integration/Console/Server/ServerDeleteCommandTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,25 +77,6 @@ function createServerDeleteCommandTester(array $existingServers = []): CommandTe
'--yes' => true,
]);

// ASSERT
$output = $tester->getDisplay();
expect($exitCode)->toBe(Command::SUCCESS)
->and($output)->toContain('deleted successfully');
});

it('deletes server when confirmation is provided', function () {
// ARRANGE
$existingServers = [
new ServerDTO('delete-me', '192.168.1.1', 22, 'root', null),
];
$tester = createServerDeleteCommandTester($existingServers);

// ACT
$exitCode = $tester->execute([
'--name' => 'delete-me',
'--yes' => true,
]);

// ASSERT
$output = $tester->getDisplay();
expect($exitCode)->toBe(Command::SUCCESS)
Expand Down Expand Up @@ -146,53 +127,31 @@ function createServerDeleteCommandTester(array $existingServers = []): CommandTe
// Display Verification
// -------------------------------------------------------------------------------

it('displays server info before confirmation', function () {
// ARRANGE
$existingServers = [
new ServerDTO('display-test', 'example.com', 2222, 'deployer', '~/.ssh/key'),
];
$tester = createServerDeleteCommandTester($existingServers);

// ACT
$tester->execute([
'--name' => 'display-test',
'--yes' => true,
]);

// ASSERT
$output = $tester->getDisplay();
expect($output)->toContain('Name:')
->and($output)->toContain('display-test')
->and($output)->toContain('Host:')
->and($output)->toContain('example.com')
->and($output)->toContain('Port:')
->and($output)->toContain('2222')
->and($output)->toContain('User:')
->and($output)->toContain('deployer')
->and($output)->toContain('Key:')
->and($output)->toContain('~/.ssh/key');
});

it('displays default SSH key message when path is null', function () {
it('displays server information before deletion', function (ServerDTO $server, array $expectedOutput) {
// ARRANGE
$existingServers = [
new ServerDTO('default-key-server', '192.168.1.1', 22, 'root', null),
];
$tester = createServerDeleteCommandTester($existingServers);
$tester = createServerDeleteCommandTester([$server]);

// ACT
$tester->execute([
'--name' => 'default-key-server',
'--name' => $server->name,
'--yes' => true,
]);

// ASSERT
$output = $tester->getDisplay();
expect($output)->toContain('Key:')
->and($output)->toContain('default')
->and($output)->toContain('~/.ssh/id_ed25519')
->and($output)->toContain('~/.ssh/id_rsa');
});
foreach ($expectedOutput as $expected) {
expect($output)->toContain($expected);
}
})->with([
'custom key path' => [
new ServerDTO('custom-key', 'example.com', 2222, 'deployer', '~/.ssh/key'),
['Name:', 'custom-key', 'Host:', 'example.com', 'Port:', '2222', 'User:', 'deployer', 'Key:', '~/.ssh/key'],
],
'default key path' => [
new ServerDTO('default-key', '192.168.1.1', 22, 'root', null),
['Name:', 'default-key', 'Host:', '192.168.1.1', 'Port:', '22', 'User:', 'root', 'Key:', 'default', '~/.ssh/id_ed25519', '~/.ssh/id_rsa'],
],
]);

it('shows command hint with correct parameters', function () {
// ARRANGE
Expand Down
24 changes: 0 additions & 24 deletions tests/Integration/Console/Server/ServerListCommandTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,30 +146,6 @@ function createServerListCommandTester(array $existingServers = []): CommandTest
->and($output)->not->toContain('default');
});

it('displays all server fields correctly', function () {
// ARRANGE
$existingServers = [
new ServerDTO('full-details', 'server.example.com', 9022, 'sysadmin', '/home/user/.ssh/key'),
];
$tester = createServerListCommandTester($existingServers);

// ACT
$exitCode = $tester->execute([]);

// ASSERT
$output = $tester->getDisplay();
expect($output)->toContain('Name:')
->and($output)->toContain('full-details')
->and($output)->toContain('Host:')
->and($output)->toContain('server.example.com')
->and($output)->toContain('Port:')
->and($output)->toContain('9022')
->and($output)->toContain('User:')
->and($output)->toContain('sysadmin')
->and($output)->toContain('Key:')
->and($output)->toContain('/home/user/.ssh/key');
});

it('lists servers in order they appear in inventory', function () {
// ARRANGE
$existingServers = [
Expand Down
18 changes: 0 additions & 18 deletions tests/Unit/Repositories/ServerRepositoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,22 +135,4 @@
// ASSERT
expect($repository->all())->toBeArray()->toBeEmpty();
});

it('loads existing servers from inventory', function () {
// ARRANGE
$inventory = mockInventoryService(true, ['servers' => [
['name' => 'web1', 'host' => '192.168.1.1', 'port' => 22, 'username' => 'root', 'privateKeyPath' => null],
['name' => 'web2', 'host' => '192.168.1.2', 'port' => 22, 'username' => 'root', 'privateKeyPath' => null],
]]);
$inventory->loadInventoryFile();
$repository = new ServerRepository();

// ACT
$repository->loadInventory($inventory);

// ASSERT
expect($repository->all())->toHaveCount(2)
->and($repository->findByName('web1'))->not->toBeNull()
->and($repository->findByName('web2'))->not->toBeNull();
});
});