feat(attribute_value): Don't target NAN, INF, -INF and > 2^53 #151

oakbani · 2018-12-10T11:10:18Z

Summary

Don't target NAN, INF or -INF doubles
Don't target Values > 2^53.
Modified is_attribute_valid to check if the number is finite.

Test plan

Issues

OASIS-3654
#146 (comment)

oakbani · 2018-12-10T11:14:01Z

Let me know if I should extend is_finite_check to revenue and numeric values within this PR. Looks like a PR on parsing revenue value is already pending for a while. #134

coveralls · 2018-12-10T11:17:14Z

Coverage increased (+0.08%) to 99.695% when pulling 3137fd4 on oakbani/-dont-target-nan-inf-1e53 into 3121437 on master.

nchilada

Thanks for putting this up! And I'm sorry, I'm not sure why I didn't see this last week.

Let me know if I should extend is_finite_check to revenue and numeric values within this PR. Looks like a PR on parsing revenue value is already pending for a while. #134

We can only effectively accept integers for revenue, so I'm thinking we should probably just close #134. 😕

I'm asking offline to figure out the exact size limits for an event's revenue/value/tags. Sorry for not having an immediate answer!

optimizely/helpers/validator.py

nchilada · 2018-12-18T03:51:26Z

tests/helpers_tests/test_validator.py

+
+    with mock.patch('optimizely.helpers.validator.is_finite_number',
+                    return_value='abc') as is_finite:
+      self.assertEqual('abc', validator.is_attribute_valid('test_attribute', 0))


Nit: I'm not sure this block is needed.

tests/helpers_tests/test_validator.py

nchilada · 2018-12-20T02:19:21Z

Let me know if I should extend is_finite_check to revenue and numeric values within this PR. Looks like a PR on parsing revenue value is already pending for a while. #134

We can only effectively accept integers for revenue, so I'm thinking we should probably just close #134. 😕

I'm asking offline to figure out the exact size limits for an event's revenue/value/tags. Sorry for not having an immediate answer!

Let's skip this after all! It's taking a while to get an answer, it's not directly related to our current epic, and we're skipping this for now in other SDKs (e.g. optimizely/objective-c-sdk#348).

…nt-target-nan-inf-1e53

oakbani · 2018-12-20T10:07:04Z

tests/helpers_tests/test_validator.py

+    self.assertTrue(validator.is_finite_number(0))
+    self.assertTrue(validator.is_finite_number(5))
+    self.assertTrue(validator.is_finite_number(5.5))
+    self.assertTrue(validator.is_finite_number(float(2**53) + 1.0))


https://stackoverflow.com/questions/1848700/biggest-integer-that-can-be-stored-in-a-double

Nit: this is really testing the behavior of floating-point arithmetic, so I'm not sure we need this assertion. But if we keep it, maybe we could include a code comment like

# float(2**53) + 1.0 evaluates to float(2**53)

so that code readers aren't confused by this scenario

nchilada

Looks good! Just one comment in the tests - I think we're testing value types instead of code branches when it comes to is_finite_number.

nchilada · 2018-12-20T21:31:40Z

tests/helpers_tests/test_validator.py

+    self.assertTrue(validator.is_finite_number(0))
+    self.assertTrue(validator.is_finite_number(5))
+    self.assertTrue(validator.is_finite_number(5.5))
+    self.assertTrue(validator.is_finite_number(float(2**53) + 1.0))


Nit: this is really testing the behavior of floating-point arithmetic, so I'm not sure we need this assertion. But if we keep it, maybe we could include a code comment like

# float(2**53) + 1.0 evaluates to float(2**53)

so that code readers aren't confused by this scenario

nchilada · 2018-12-20T21:38:23Z

tests/helpers_tests/test_validator.py

+    self.assertFalse(validator.is_finite_number(int(2**53) + 1))
+    self.assertFalse(validator.is_finite_number(-int(2**53) - 1))
+    self.assertFalse(validator.is_finite_number(float(2**53) + 2.0))
+    self.assertFalse(validator.is_finite_number(-float(2**53) - 2.0))


Oh yeah, thanks for figuring out the need for + 2!

nchilada · 2018-12-20T22:07:21Z

tests/helpers_tests/test_condition.py

+                       side_effect=side_effect) as is_finite:
+        self.assertIsNone(evaluator.evaluate(0))
+      is_finite.assert_called_with(long(49))
+


Ooh, I like the idea of using side_effect here. Can we go farther with this and test that is_finite only needs to reject the condition value or the user attribute value in order to stop condition evaluation?

Something like

def test_greater_than__calls_is_finite_number(self): evaluator = condition_helper.CustomAttributeConditionEvaluator( gt_int_condition_list, {'meters_travelled': 48.1} ) def is_finite_number__rejecting_condition_value(value): if value == 48: return False return True with mock.patch('optimizely.helpers.validator.is_finite_number', side_effect=is_finite_number__rejecting_condition_value): self.assertIsNone(evaluator.evaluate(0)) def is_finite_number__rejecting_user_attribute_value(value): if value == 48.1: return False return True with mock.patch('optimizely.helpers.validator.is_finite_number', side_effect=is_finite_number__rejecting_user_attribute_value): self.assertIsNone(evaluator.evaluate(0)) def is_finite_number__accepting_both_values(value): return True with mock.patch('optimizely.helpers.validator.is_finite_number', side_effect=is_finite_number__accepting_both_values): self.assertIsNone(evaluator.evaluate(0))

I don't think we need to test ints, longs, and floats separately... not here, at least... we can probably get away with test_greater_than__calls_is_finite_number, test_less_than__calls_is_finite_number, and maybe test_exact__given_number_values__calls_is_finite_number. If that makes sense.

@oakbani @aliabbasrizvi what do you think?

Nice suggestion. Revised accordingly.

Thanks @oakbani! test_greater_than__calls_is_finite_number and test_less_than__calls_is_finite_number look good. Can you also update test_exact__given_number_values__calls_is_finite_number to test the same three cases instead of only testing two cases?

msohailhussain · 2018-12-21T22:33:48Z

optimizely/helpers/validator.py


  Args:
    value: Value to be validated.

  Returns:
-    Boolean: True if value is a number and not NAN, INF or -INF else False.
+    Boolean: True if value is a number and not NAN, INF, -INF or greater than 2^53 else False.


nit: or absolute limit of 2^53

Late to the party, but wouldn't I want this limit for Double type variables and Integer (sys.maxint) for Integer variables?

@nchilada ^

I think I get it. I am mixing feature variables here. Apologies.

@aliabbasrizvi it's true that Python integers can have larger values than 2^53, but they're passed to the backend as JSON Numbers which are best limited to 2^53. Does that clarify why we limit integers too?

aliabbasrizvi

More or less looks good. Minor feedback around docstrings and naming.

Please address and then @nchilada can sign off as well.

aliabbasrizvi · 2018-12-22T00:02:01Z

tests/helpers_tests/test_condition.py

+        exact_int_condition_list, {'lasers_count': 9000}
+      )
+
+    with mock.patch('optimizely.helpers.validator.is_finite_number',


nit. Please be explicit in naming the mocks. ...as mock_is_finite

aliabbasrizvi · 2018-12-22T00:24:38Z

tests/helpers_tests/test_condition.py

@@ -286,6 +282,27 @@ def test_exact_float__returns_null__when_no_user_provided_value(self):

    self.assertIsNone(evaluator.evaluate(0))

+  def test_exact__given_number_values__calls_is_finite_number(self):
+    """ Returns True if is_finite_number returns True. Returns None if is_finite_number returns False. """


nit. Looks like an incorrect statement? What Returns? evaluator right?

Perhaps update Test that CustomAttributeConditionEvaluator.evaluator returns True ....

Please apply this comment to subsequent tests as well.

nchilada

One last test request! Thanks so much, this is looking good

nchilada · 2018-12-26T21:28:00Z

tests/helpers_tests/test_condition.py

+                       side_effect=side_effect) as is_finite:
+        self.assertIsNone(evaluator.evaluate(0))
+      is_finite.assert_called_with(long(49))
+


Thanks @oakbani! test_greater_than__calls_is_finite_number and test_less_than__calls_is_finite_number look good. Can you also update test_exact__given_number_values__calls_is_finite_number to test the same three cases instead of only testing two cases?

oakbani · 2018-12-27T04:48:40Z

tests/helpers_tests/test_condition.py

-                    return_value=True) as mock_is_finite:
-      self.assertTrue(evaluator.evaluate(0))
-    mock_is_finite.assert_called_with(9000)
+                    side_effect=[False, True]) as mock_is_finite:


@nchilada In exact match need to send exactly same values to ensure that it's the mocked is_finite that is really driving the result. Therefore, can't make a value-base side-effect method as in gt/lt methods.
This returns False on 1st call and True on 2nd.

Ah, you're right. Good solution. Thanks @oakbani!

nchilada · 2018-12-27T06:17:44Z

tests/helpers_tests/test_condition.py

-                    return_value=True) as mock_is_finite:
-      self.assertTrue(evaluator.evaluate(0))
-    mock_is_finite.assert_called_with(9000)
+                    side_effect=[False, True]) as mock_is_finite:


Ah, you're right. Good solution. Thanks @oakbani!

feat(attribute_value): Don't target NAN, INF, -INF and Integers > 1e53

7320a3c

oakbani requested review from aliabbasrizvi, nchilada and msohailhussain December 10, 2018 11:10

nit. blank lines

c532525

nchilada mentioned this pull request Dec 18, 2018

Parses revenue value and logger integration #134

Closed

nchilada suggested changes Dec 18, 2018

View reviewed changes

Merge remote-tracking branch 'remotes/origin/master' into oakbani/-do…

5b96ff2

…nt-target-nan-inf-1e53

oakbani commented Dec 20, 2018

View reviewed changes

🖊️ Revise number

66159a8

oakbani changed the title ~~feat(attribute_value): Don't target NAN, INF, -INF and Integers > 1e53~~ feat(attribute_value): Don't target NAN, INF, -INF and > 2^53 Dec 20, 2018

nchilada suggested changes Dec 20, 2018

View reviewed changes

tests: refact

da583c8

msohailhussain reviewed Dec 21, 2018

View reviewed changes

aliabbasrizvi approved these changes Dec 22, 2018

View reviewed changes

docstrings/namings

24a6350

nchilada suggested changes Dec 26, 2018

View reviewed changes

Update exact_match testcase

8c6fbb4

oakbani commented Dec 27, 2018

View reviewed changes

nchilada approved these changes Dec 27, 2018

View reviewed changes

Merge branch 'master' into oakbani/-dont-target-nan-inf-1e53

3137fd4

aliabbasrizvi merged commit 2c35eb8 into master Dec 28, 2018

aliabbasrizvi deleted the oakbani/-dont-target-nan-inf-1e53 branch December 28, 2018 21:57

feat(attribute_value): Don't target NAN, INF, -INF and > 2^53 #151

feat(attribute_value): Don't target NAN, INF, -INF and > 2^53 #151

Uh oh!

Conversation

oakbani commented Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Issues

Uh oh!

oakbani commented Dec 10, 2018

Uh oh!

coveralls commented Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nchilada left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nchilada commented Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchilada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchilada Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchilada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oakbani commented Dec 10, 2018 •

edited

Loading

coveralls commented Dec 10, 2018 •

edited

Loading

nchilada commented Dec 20, 2018 •

edited

Loading

nchilada Dec 20, 2018 •

edited

Loading