Skip to content

Conversation

warber
Copy link
Contributor

@warber warber commented Oct 22, 2024

This PR

  • implements an idea how to improve possibility to write highly parallel unit tests.

Related Issues

Fixes: #293

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 70.21277% with 14 lines in your changes missing coverage. Please review.

Project coverage is 81.45%. Comparing base (0b13f8a) to head (853d9f4).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/openfeaturetest/testprovider.go 70.21% 11 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #295      +/-   ##
==========================================
- Coverage   81.89%   81.45%   -0.44%     
==========================================
  Files          10       11       +1     
  Lines        1215     1262      +47     
==========================================
+ Hits          995     1028      +33     
- Misses        199      210      +11     
- Partials       21       24       +3     

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

@warber warber force-pushed the test/experimental-test-aware-provider branch from 209bd88 to 853d9f4 Compare October 22, 2024 14:45
@@ -0,0 +1,104 @@
package openfeaturetest
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 guess we need to come up with a better place for this ? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

the question is, if we want this to be part of the sdk or rather put this as a separate package in the sdk-contrib - like we did in java for the junit extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then to me this sounds like contrib would be the better place. @toddbaert ?

Copy link
Member

Choose a reason for hiding this comment

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

Since there's no external deps, I think we should include this in the SDK.

return value
}

func getGoroutineID() uint64 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not perfect but does the trick of getting the current gid from the runtime

Copy link

@natenho natenho Oct 30, 2024

Choose a reason for hiding this comment

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

@warber to be honest it is a very hacky way of dealing with that. Maybe we can think of a more elegant solution since the go routine id is intentionally not available.

Copy link
Member

@toddbaert toddbaert Oct 30, 2024

Choose a reason for hiding this comment

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

@warber to be honest it is a very hacky way of dealing with that. Maybe we can think of a more elegant solution since the go routine id is intentionally not available.

I agree it's hacky, however, there are multiple relatively popular packages that use this same mechanism, and we are only using it for testing. I would not advocate for including this in production code, but I think it's acceptable to dodge our singleton issue for tests, and include it only in a testing package.

IMO this is a net improvement. I'm open to other solutions but at the end of the day, we need some transparent piece of data (at least an id) to propagate from the test to the production code; I'm not aware of any other alternative.

Copy link

Choose a reason for hiding this comment

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

ok, I can agree with that

Added simple version of a "TestAware" provider that is managing a TestName=>FeatureProvider map and knows which feature provider to delegate the methods calls for a given test.

Signed-off-by: Bernd Warmuth <bernd.warmuth@dynatrace.com>
@warber warber force-pushed the test/experimental-test-aware-provider branch from 853d9f4 to 356940d Compare October 23, 2024 12:27
// `SetProvider` method.
type TestAwareProvider struct {
openfeature.NoopProvider
providers *sync.Map
Copy link
Member

Choose a reason for hiding this comment

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

My only current issue with this approach is cleaning up this map. It would be great if we could have some ergonomic mechanism to REMOVE things from this map, eventually. Perhaps something a user could clean-up with a defer statement in whatever scope they need.

Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

I approve of this approach. This is my only blocker.

Signed-off-by: Bernd Warmuth <bernd.warmuth@dynatrace.com>
@warber warber marked this pull request as ready for review November 1, 2024 10:38
@warber warber requested a review from a team as a code owner November 1, 2024 10:38
@toddbaert
Copy link
Member

toddbaert commented Nov 4, 2024

@warber Looks good, I have proposed one more additional ergonomic change I'd like your opinion on. Fundamentally it works the exact same way, it just changes the API a bit so that users are thinking in terms of flags for a test scope, not a provider for a test scope: warber#1

If you agree, I think we should just add a quick blurb about this in the README and then merge.

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert toddbaert force-pushed the test/experimental-test-aware-provider branch from b78ffd7 to 9c5de09 Compare November 5, 2024 13:32
@toddbaert
Copy link
Member

toddbaert commented Nov 5, 2024

@warber after you pointed out the deprecated packaging, I moved this to openfeature/testing: 9c5de09

This is consistent with go's testing package naming, I think.

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert toddbaert changed the title test: improve parallel testing feat: TestProvider for easy, parallel-safe testing Nov 5, 2024
@toddbaert toddbaert merged commit 3e3d0b1 into open-feature:main Nov 5, 2024
7 checks passed
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.

[BUG] Race condition when testing with memprovider
5 participants