Skip to content

chore(unit-tests): Implements unit tests for src/libcommon with the help of OpenCode#1465

Open
luc-guyot-infomaniak wants to merge 21 commits intodevelopfrom
trello-1436-generate-libcommon-unit-tests-with-the-help-of-opencode
Open

chore(unit-tests): Implements unit tests for src/libcommon with the help of OpenCode#1465
luc-guyot-infomaniak wants to merge 21 commits intodevelopfrom
trello-1436-generate-libcommon-unit-tests-with-the-help-of-opencode

Conversation

@luc-guyot-infomaniak
Copy link
Contributor

No description provided.

@luc-guyot-infomaniak luc-guyot-infomaniak force-pushed the trello-1436-generate-libcommon-unit-tests-with-the-help-of-opencode branch 2 times, most recently from 484c291 to 6513579 Compare February 6, 2026 08:26
@luc-guyot-infomaniak luc-guyot-infomaniak force-pushed the trello-1436-generate-libcommon-unit-tests-with-the-help-of-opencode branch 2 times, most recently from 1c1e2a6 to 66fddd4 Compare February 17, 2026 13:48
@luc-guyot-infomaniak luc-guyot-infomaniak marked this pull request as ready for review February 18, 2026 07:52
@luc-guyot-infomaniak luc-guyot-infomaniak requested a review from a team as a code owner February 18, 2026 07:52
Copy link
Contributor

@herve-er herve-er left a comment

Choose a reason for hiding this comment

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

It looks like some of the previous tests are no longer included (see my comments).

Regarding the changes in KeychainManager:

I totally see the benefit of IKeychainStore for testing purposes, but I feel like the keychain handling is becoming a bit overcomplicated.

The current chain (mixing inheritance and members) is:

IKeychainStore → KeychainStore → KeychainManager → KeychainManagerSingleton

KeychainManager mainly acts as a wrapper around KeychainStore methods, adding logging and Sentry reporting. However, logging is already handled everywhere we call KeychainManager(Singleton)::Instance()->…, so this feels somewhat redundant. Its added value is storing _service and _package, but this could probably be handled directly within KeychainStore.

KeychainManagerSingleton simply holds an instance of KeychainManager, effectively acting as a makeshift dependency injection mechanism. I would suggest waiting a proper DI approach instead of relying on these kinds of workarounds.

This is an interesting subject, we can discuss it anytime tomorrow 😄

@luc-guyot-infomaniak luc-guyot-infomaniak force-pushed the trello-1436-generate-libcommon-unit-tests-with-the-help-of-opencode branch from 66fddd4 to b583f17 Compare March 2, 2026 13:46
…ger with the feature of a setter for a custom store used in tests as a mock object
@luc-guyot-infomaniak luc-guyot-infomaniak force-pushed the trello-1436-generate-libcommon-unit-tests-with-the-help-of-opencode branch from b583f17 to 3023589 Compare March 2, 2026 14:14
@luc-guyot-infomaniak luc-guyot-infomaniak changed the title [TRELLO-1436] chore(unit-tests): implements unit tests for src/libcommon with the help of OpenCode chore(unit-tests): Implements unit tests for src/libcommon with the help of OpenCode Mar 2, 2026
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 2, 2026

@luc-guyot-infomaniak
Copy link
Contributor Author

luc-guyot-infomaniak commented Mar 2, 2026

KeychainManagerSingleton simply holds an instance of KeychainManager, effectively acting as a makeshift dependency injection mechanism. I would suggest waiting a proper DI approach instead of relying on these kinds of workarounds.

This is an interesting subject, we can discuss it anytime tomorrow 😄

The KeyManager was originally a singleton and changing this design has implications which go, in my opinion, much beyond the scope of this PR. With my current proposal, the singleton pattern is preserved while DI is achieved through inheritance by mocking the IKeychainStore.

What the proper DI approach that you envision?

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.

2 participants