Skip to content
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.

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

feat: isolate interfaces from SDK to improve testability #268

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Kavindu-Dodan
Copy link
Contributor

@Kavindu-Dodan Kavindu-Dodan commented Apr 29, 2024

This PR

Partially address #266 by providing interface contracts for the SDK. There are no breaking changes in this change

I have introduced IEvaluation interface, which represents the general OpenFeatuer API layer [1]. Before this change, there was no such interface so mocking API interactions was impossible. To obtain the singleton instance of IEvaluation interface, I have introduced GetApiInstance() helper.

Consider the example below,

// get the global instance that may be injected into testable components 
apiInstance := openfeature.GetApiInstance()

// use for OpenFeature operations 
apiInstance.SetProvider(myProvider)
apiInstance.AddHandler(....)

In the future, we can update our documentation to make API instance based operations the preferred interaction method.

Further, IEvaluation exposes client creation methods GetClient() & GetNamedClient(clientName string) (similar to spec [2]). These methods return IClient instances, making it easy to test OpenFeature client related operations.

apiInstance := openfeature.GetApiInstance()

// IClient instance, which can be mocked when injected into preferred locations 
client := apiInstance.GetClient()

[1] - https://openfeature.dev/specification/sections/flag-evaluation
[2] - https://openfeature.dev/specification/sections/flag-evaluation#creating-clients

Copy link

codecov bot commented Apr 29, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 81.92%. Comparing base (a9e19dd) to head (30e3e4e).

Files Patch % Lines
openfeature/client.go 72.22% 5 Missing ⚠️
openfeature/openfeature_api.go 93.33% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #268      +/-   ##
==========================================
+ Coverage   81.74%   81.92%   +0.17%     
==========================================
  Files          10       10              
  Lines         964      979      +15     
==========================================
+ Hits          788      802      +14     
  Misses        156      156              
- Partials       20       21       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Kavindu-Dodan Kavindu-Dodan changed the title feat: [WIP] isolate interfaces from SDK & derive test utilities feat: [WIP] isolate interfaces from SDK to improve testability May 1, 2024
@Kavindu-Dodan Kavindu-Dodan changed the title feat: [WIP] isolate interfaces from SDK to improve testability feat: isolate interfaces from SDK to improve testability May 1, 2024
@Kavindu-Dodan Kavindu-Dodan marked this pull request as ready for review May 1, 2024 16:25
@Kavindu-Dodan Kavindu-Dodan requested a review from a team as a code owner May 1, 2024 16:25
@Kavindu-Dodan Kavindu-Dodan force-pushed the feat/improve-testability branch 2 times, most recently from a6c2c55 to 1af6cf0 Compare May 1, 2024 16:50
// evaluationAPI wraps OpenFeature evaluation API functionalities
type evaluationAPI struct {
// ofApiImpl is an internal reference interface extending IOFApi
type ofApiImpl interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not clear on why this intermediate interface is necessary - can you clarify why it exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I introduced this to support internal work and avoid direct access to singleton instance from other components like Client. For example, a client instance needs to obtain the provider associate with its domain, so it rely on ForEvaluation method exposed by this internal interface.

// registerClientHandler registers a client level handler
func (e *eventExecutor) registerClientHandler(clientName string, t EventType, c EventCallback) {
// RegisterClientHandler registers a client level handler
func (e *EventExecutor) RegisterClientHandler(clientName string, t EventType, c EventCallback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe name this AddClientHandler to reflect the relationship to AddHandler (i.e. the same except scoped), or perhaps rename AddHandler above as RegisterHandler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will rename to AddClientHandler. Regarding AddHandler, this is already public so I will avoid a breaking change and keep it there.

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
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.

None yet

2 participants