test: improve unit tests coverage for AccountService (Infection)

[diogopaz]

Part of #8053

📝 Summary

This PR adds extensive PHPUnit test coverage to AccountServiceTest to eliminate escaped mutants and improve the mutation testing score with Infection, as part of #8053.

🧪 How to test

Run the focused unit test suite for AccountService:

composer test:unit -- --filter AccountServiceTest

Optionally run Infection on this specific pair to verify mutant coverage:

vendor-bin/infection/vendor/bin/infection --filter=AccountService

⚙️ API / Back‑end changes

• Unit tests added to cover validateCreateToSign, getFileByUuid, createToSign, saveVisibleElement(s), uploadPfx, updatePfxPassword, readPfxData, and config fallbacks.
• Unit and/or integration tests added – required for backend changes

✅ Checklist

• I have read and followed the contribution guide.

🤖 AI (if applicable)

• The content of this PR was partially or fully generated using AI

[welcome]

Thanks for opening your first pull request in this repository! ✌️

[vitormattos]

@diogopaz the problems in the unit tests aren't caused by your PR. I'll fix this in a separate PR.

But have linter issues that you need to solve:

https://github.com/LibreSign/libresign/actions/runs/33325571740/job/100018743935?pr=8142

[vitormattos]

Once you fix the linter issue and push the changes, the unit tests will work fine.

[diogopaz]

Hi @vitormattos,

Sorry for the delay! I've fixed the linter issues (missing imports, multiline array trailing commas, and formatting) and updated the commit.

Thanks for the review!

[vitormattos]

I checked the PHPUnit failures.

There are two different groups of problems:

1. The IUserFolder errors in ValidateHelperTest and SignFileServiceTest are not caused by this PR. They come from tests outside the changed file and appear in the Nextcloud master jobs. We should handle this compatibility change separately.

2. The failures in AccountServiceTest are from the tests added in this PR and need to be fixed here before merge. I added inline comments for the main cases.

So you do not need to fix the unrelated IUserFolder failures as part of this PR.

[vitormattos]

I think we should test one more case before making this cache behavior part of the expected behavior.

AccountService caches signRequest, fileData and fileToSign, but the cache does not store which UUID the data belongs to.

This test calls getFileByUuid() twice with the same UUID, so it does not detect what happens with different UUIDs.

Could we also test something like:

$service->getFileByUuid('uuid-a');
$result = $service->getFileByUuid('uuid-b');

and verify that the result belongs to uuid-b?

If this fails with the current implementation, we found a bug and should fix it instead of protecting this behavior with the mutation test. This is also tracked in #8328.

[vitormattos]

This expectation does not match the current behavior.

These elements only contain elementId and starred. They do not contain file, so updateFileOfVisibleElement() returns before calling folderService->getFileByNodeId().

This is also what the failing PHPUnit job shows: the method was expected 2 times but was called 0 times.

Could you remove this filesystem setup and keep this test focused on the starred update behavior?

[vitormattos]

There are a few related failures in the new visible-element tests that should be fixed before merge.

In this test, saveVisibleElement() calls both updateFileOfVisibleElement() and updateDataOfVisibleElement(). Each method calls userElementMapper->findOne(), so expecting findOne() only once makes the test fail.

There are two similar problems below:

• getStarred() returns a boolean here, so assertSame(1, ...) fails with true. Please assert the actual type/value returned by the entity.
• The invalid URL tests call saveVisibleElement() without preparing the folder needed by the session path. The code fails with newFile() on null before it reaches the URL validation that the test wants to check.

Please adjust the setup so each test reaches the behavior it is meant to test. The invalid URL / MIME / empty body cases may also fit well in one data provider because they test the same rule with different inputs.

[vitormattos]

This test also shows why #8328 is useful: testing one account operation requires setup for account creation, identification, email and certificate handling.

I would keep the architecture refactor outside this PR, but we should avoid adding more test complexity only to improve the Infection score.

[vitormattos]

@diogopaz, can you rebase your branch on top of main?

I fixed part of the failing tests in this PR:

• test: use IUserFolder mocks for Nextcloud 36 #8329

[maia-andre]

Heads-up after #8366 was merged (this morning): AccountService no longer receives ValidateHelper. AccountServiceTest on main now mocks IdentityDocumentValidator ($this->identityDocumentValidator) and FileInputValidator ($this->fileInputValidator) and passes them to the constructor; the $this->validateHelper property and the use OCA\Libresign\Helper\ValidateHelper; import are gone.

After rebasing, five expectations in this PR will reference a property that no longer exists. The one-to-one mapping against current main:

This PR	On main
$this->validateHelper->method('userCanApproveValidationDocuments')->with(null, false)	$this->identityDocumentValidator->method('userCanApproveValidationDocuments')->with(null, false)
$this->validateHelper->method('userCanApproveValidationDocuments')->with($user, false) (×2)	$this->identityDocumentValidator->method(...)->with($user, false)
$this->validateHelper->expects($this->once())->method('validateBase64')->with('fake_png_data', ValidateHelper::TYPE_VISIBLE_ELEMENT_USER)	$this->fileInputValidator->expects($this->once())->method('validateBase64')->with('fake_png_data', FileInputValidator::TYPE_VISIBLE_ELEMENT_USER)

The production calls are lib/Service/AccountService.php:218 and :354 (identityDocumentValidator->userCanApproveValidationDocuments($user, false)) and :522/:525 (fileInputValidator->validateBase64(..., FileInputValidator::TYPE_VISIBLE_ELEMENT_USER)). The imports for both validators are already in the test file on main, so the use ValidateHelper line can simply be dropped. Everything else in the PR is additive and should not conflict beyond the use block.

Sorry for the extra step — happy to review the rebase if useful.