Skip to content

Conversation

thomaszurkan-optimizely
Copy link
Contributor

Summary

  • Added user context and create off of optimizely. Added unit tests as well.

Test plan

Unit tests added

@coveralls
Copy link

coveralls commented Nov 9, 2020

Coverage Status

Coverage decreased (-0.2%) to 96.136% when pulling 61fe356 on decideApiUserContext into edf5528 on master.

@@ -0,0 +1,34 @@
# Copyright 2019, Optimizely
Copy link
Contributor

Choose a reason for hiding this comment

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

2020

@@ -0,0 +1,84 @@
#
Copy link
Contributor

Choose a reason for hiding this comment

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

Omit this line.

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

Looks good.
Should add mutex control for attributes. More tests suggested.

Returns:
None
"""
self.user_attributes[attribute_key] = attribute_value
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a mutex control for attributes read/write?
"setAttributes" and attribute -reading for decide can happen in other threads. See C#-sdk as a good reference (optimizely/csharp-sdk#253)

uc.set_attribute("key", "value")
self.assertEqual(uc.user_attributes["key"], "value", "should have added attribute")
uc.set_attribute("key", "value2")
self.assertEqual(uc.user_attributes["key"], "value2", "should have new attribute")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have more tests:

  • construct with non-empty attributes
  • setAttribute can override the init attributes
  • attributes not changed once constructed when the caller copy is updated
Copy link
Contributor

Choose a reason for hiding this comment

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

Done in 314

@jaeopt jaeopt closed this Feb 1, 2021
@jaeopt
Copy link
Contributor

jaeopt commented Feb 1, 2021

Merged with #309

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants