Add support for unit testing for user context #271
Conversation
…re to english, as for systems on others cultures, some tests were failing.
|
Thanks Daniel. Can you please sign this CLA and update this branch, so we can review & merge this PR? |
Hi, I already did before creating PR. probably I'm around your records |
Supports mocking OptimizelyUserContext on frameworks as almost all of them require an interface.
daniel-pons
force-pushed
the
daniel-pons/support-ut-for-user-context
branch
from
97b7f5c to
2b06f71
Compare
I think I fixed what was requested. However I'm getting some kind of error when running the integration tests. Many thanks. |
daniel-pons
requested review from
msohailhussain
and removed request for
a team
Thanks I will take care of it CI stuff. |
| @@ -41,7 +41,7 @@ public interface IOptimizely | |||
| /// <param name="userId">The user ID to be used for bucketing.</param> | |||
| /// <param name="userAttributes">The user's attributes</param> | |||
| /// <returns>OptimizelyUserContext | An OptimizelyUserContext associated with this OptimizelyClient.</returns> | |||
| OptimizelyUserContext CreateUserContext(string userId, UserAttributes userAttributes = null); | |||
| IOptimizelyUserContext CreateUserContext(string userId, UserAttributes userAttributes = null); | |||
There was a problem hiding this comment.
this will break existing functionality.
e.g. someone used like this.
OptimizelyUserContext ouc = obj.CreateUserContext(...) /// value can't be set when returning IOptimizelyUserContext.
We should return to OptimizelyUserContext.
There was a problem hiding this comment.
Well, the idea of the PR is to get the interface, as using the explicit type when declaring is in disuse nowadays. Usually code analyzers mark this as warnings. EG:
- https://www.jetbrains.com/help/resharper/Using_var_Keyword_in_Declarations.html
- https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0007-ide0008
Then, is just a matter of replacing:
var ouc = obj.CreateUserContext(...);
I think that, the benefits of being able to write unit tests is by far higher for the majority of developers, than the cost of replacing the explicit type by var.
What to you thnik?.
Thanks
There was a problem hiding this comment.
In fact, if the coding standards don't allow the usage of var, is just a matter of using the interface instead:
IOptimizelyUserContext ouc = obj.CreateUserContext(...)
There was a problem hiding this comment.
In fact, if the coding standards don't allow the usage of var, is just a matter of using the interface instead:
IOptimizelyUserContext ouc = obj.CreateUserContext(...)
Unfortunately it's not just a matter of coding standards, this would technically require a breaking change to implement in the current version.
We agree this is a worthwhile enhancement for the next major version of the SDK and will add it to our backlog for when that next release is planned.
What we can do in the interim is provide a virtual that will get the user context provided, which can then be overridden for your testing purposes.
Let us know if you think that would be a good stop gap until we can provide the interface change.
There was a problem hiding this comment.
Yes indeed, this is a breaking change, if you guys consider it useful and want to include this on the next major release, is perfect.
Then, we can wait, I prefer to close the PR and wait.
Many thanks.
There was a problem hiding this comment.
But well, you are right, making them virtual is just a nice quick win, and that will allow the mocking library, to intercept the calls and set expectations.
There was a problem hiding this comment.
I created the PR with the virtual keyword. It's here: #280
It also includes the fix for unit tests running on non English systems.
I'll then, close this PR.
Many thanks!.
| @@ -53,7 +53,7 @@ public class OptimizelyDecision | |||
| /// <summary> | |||
| /// user context for which the decision was made. | |||
| /// </summary> | |||
| public OptimizelyUserContext UserContext { get; private set; } | |||
| public IOptimizelyUserContext UserContext { get; private set; } | |||
There was a problem hiding this comment.
and this one as well, please revert to OptimizelyUserContext
There was a problem hiding this comment.
The same as before
|
@daniel-pons I tested your changes but that code will break existing code, can you please revert those. I have pointed out as well. |
|
I created the PR with the virtual keyword. It's here: #280 It also includes the fix for unit tests running on non English systems. I'll then, close this PR. Many thanks!. |
Summary
The following versions of Optimizely C# SDK would use the methods defined on the class OptimizelyUserContext to perform some processes, E.G: Decide, TrackEvent, DecideAll, etc...
Them, it is required on the vast majority of unit testing frameworks for mocking this object to have an interface for it.
This PR creates the interface IOptimizelyUserContext for doing so.
Also fixes a problem with development environments with a culture different from english.