latest fix #2

ozayr-zaviar · 2020-08-31T13:59:01Z

Summary

Testcases revise for semver

oakbani

Please fix lint issues and address comments

oakbani · 2020-08-31T14:01:10Z

optimizely/helpers/condition.py

+                    self._get_condition_json(0), type(user_version), user_version
+                )
+            )
+            return None


We don't need to put this here because

We have already verified these in all evaluators that call this method.

This method doesn't have attribute name and index to log the error.

oakbani · 2020-08-31T14:02:16Z

tests/helpers_tests/test_condition.py

@@ -29,14 +29,18 @@
 semver_equal_2_0_condition_list = [['Android', "2.0", 'custom_attribute', 'semver_eq']]
 semver_equal_2_0_1_beta_condition_list = [['Android', "2.0.1-beta", 'custom_attribute', 'semver_eq']]
 semver_greater_than_2_0_0_condition_list = [['Android', "2.0.0", 'custom_attribute', 'semver_gt']]
+semver_greater_than_2_0_condition_list = [['Android', "2.0", 'custom_attribute', 'semver_gt']]


Remove all semver conditions from here. Instead declare it as a local variable inside it's respective method.

oakbani · 2020-08-31T14:03:05Z

tests/helpers_tests/test_condition.py

+                semver_equal_2_0_condition_list, {'Android': user_version}, self.mock_client_logger)
+            result = evaluator.evaluate(0)
+            custom_err_msg = "Got {} in result. Failed for user version: {}".format(result, user_version)
+            self.assertEquals(result, True, custom_err_msg)


nit. Use assertTrue or assertStrictTrue instead

oakbani · 2020-08-31T14:04:06Z

tests/helpers_tests/test_condition.py


-    def test_evaluate__returns_exception__when_user_provided_version_is_invalid7(self):
+    def test_evaluate__returns_false__when_user_version_matches_target_version(self):
+        user_versions = ['2.9', '1.9']


nit. does not match

oakbani · 2020-08-31T14:05:41Z

tests/helpers_tests/test_condition.py

-        condition_helper.CustomAttributeConditionEvaluator(
-            semver_equal_2_0_condition_list, {'Android': "3.90"}, self.mock_client_logger
-        )
+    def test_evaluate__returns_true__when_user_version_less_than_or_equal_to_target_version(self):


rename all tests to something that shows the exact evaluator being used.
test_semver_eq__returns _true
test_semver_gt__returns_false

oakbani · 2020-08-31T14:08:05Z

tests/helpers_tests/test_condition.py

+            ('2', '2.12'),
+            ('2', '2.785.13')
+        ]
+        for user_version, target_version in versions:


target_version should come before user_version. Even compare method accepts target_version as first arg. This is mis leading.
Keep target before user in list, in iteration and passing arg

oakbani · 2020-08-31T14:09:00Z

tests/helpers_tests/test_condition.py

@@ -431,23 +363,23 @@ def test_exists__returns_true__when_user_provided_value_is_boolean(self):

        self.assertStrictTrue(evaluator.evaluate(0))

-    def test_exact_string__returns_true__when_user_provided_value_is_equal_to_condition_value(self,):
+    def test_exact_string__returns_true__when_user_provided_value_is_equal_to_condition_value(self, ):


remove all these fixes below from the diff

oakbani

few more changes

oakbani · 2020-08-31T15:08:34Z

tests/helpers_tests/test_condition.py

@@ -25,22 +25,16 @@
 doubleCondition = ['pi_value', 3.14, 'custom_attribute', 'exact']

 semver_equal_2_0_0_condition_list = [['Android', "2.0.0", 'custom_attribute', 'semver_eq']]
-semver_equal_2_condition_list = [['Android', "2", 'custom_attribute', 'semver_eq']]
-semver_equal_2_0_condition_list = [['Android', "2.0", 'custom_attribute', 'semver_eq']]
 semver_equal_2_0_1_beta_condition_list = [['Android', "2.0.1-beta", 'custom_attribute', 'semver_eq']]


I see other variables starting with prefix semver_ here. Are they being used? If not, we should remove them.

oakbani · 2020-08-31T15:09:39Z

tests/helpers_tests/test_condition.py

        ]
-        for user_version, target_version in versions:
+        for target_version, user_version in versions:
            evaluator = condition_helper.CustomAttributeConditionEvaluator(
                semver_greater_than_2_0_condition_list, {'Android': user_version}, self.mock_client_logger)
            result = evaluator.compare_user_version_with_target_version(user_version, target_version)


you are still passing in the wrong order

oakbani · 2020-08-31T15:09:51Z

tests/helpers_tests/test_condition.py

        ]
-        for user_version, target_version in versions:
+        for target_version, user_version in versions:
            evaluator = condition_helper.CustomAttributeConditionEvaluator(
                semver_greater_than_2_0_condition_list, {'Android': user_version}, self.mock_client_logger)
            result = evaluator.compare_user_version_with_target_version(user_version, target_version)


and below as well

…into uzair/semver

oakbani

nits

oakbani · 2020-08-31T15:27:38Z

tests/helpers_tests/test_condition.py

@@ -184,7 +172,7 @@ def test_evaluate__returns_false__when_user_version_greater_than_or_equal_to_tar
            custom_err_msg = "Got {} in result. Failed for user version: {}".format(result, user_version)
            self.assertFalse(result, custom_err_msg)

-    def test_evaluate__returns_true__when_user_version_less_than_target_version(self):
+    def test_semver_le__returns_true(self):


oakbani · 2020-08-31T15:27:45Z

tests/helpers_tests/test_condition.py

@@ -194,7 +182,7 @@ def test_evaluate__returns_true__when_user_version_less_than_target_version(self
            custom_err_msg = "Got {} in result. Failed for user version: {}".format(result, user_version)
            self.assertTrue(result, custom_err_msg)

-    def test_evaluate__returns_false__when_user_version_less_than_target_version(self):
+    def test_semver_le__returns_false(self):


oakbani

Fix test failures and lint fixes. And we should be good to go

…into uzair/semver

oakbani

Please fix this

oakbani · 2020-09-03T15:12:30Z

optimizely/helpers/condition.py

+                return True
+            else:
+                return False
+        else:


You can remove both elses. And just use return False in the end.

oakbani · 2020-09-03T15:12:44Z

optimizely/helpers/condition.py

+                return True
+            else:
+                return False
+        else:


latest fix

bcb6a6c

oakbani requested changes Aug 31, 2020

View reviewed changes

uzair-folio3 and others added 2 commits August 31, 2020 20:02

comments addressed

7429ec1

Delete changes.patch

ba85f10

oakbani requested changes Aug 31, 2020

View reviewed changes

uzair-folio3 added 2 commits August 31, 2020 20:24

global semver variable removed and semver test renamed

08ee35c

Merge branch 'uzair/semver' of https://github.com/oakbani/python-sdk …

4b6e949

…into uzair/semver

oakbani requested changes Aug 31, 2020

View reviewed changes

uzair-folio3 added 2 commits August 31, 2020 20:32

Update test_condition.py

b9df987

Update test_condition.py

64a9745

oakbani requested changes Aug 31, 2020

View reviewed changes

uzair-folio3 and others added 14 commits August 31, 2020 20:50

Update test_condition.py

e3cfb04

Update test_condition.py

1d5a70a

semver comparision redesign, new test cases added

ec72cff

semver comparison redesign, new test cases added

b5a18ca

Merge branch 'uzair/semver' of https://github.com/oakbani/python-sdk …

d498ee8

…into uzair/semver

semver new validations added with test cases and comment addressed

c7b313c

updated according to previous implementation

fd0cef4

Update test_condition.py

8a11879

Merge branch 'amna/semver' into uzair/semver

a5b9eef

Update condition.py

7100cce

Update condition.py

07aaa35

Update condition.py

0aaa3a9

Update condition.py

a3bd7fc

Merge branch 'amna/semver' into uzair/semver

953bb4b

oakbani reviewed Sep 3, 2020

View reviewed changes

uzair-folio3 added 4 commits September 3, 2020 20:17

Update condition.py

e86061b

Update condition.py

7df804e

Update condition.py

be33a39

Update condition.py

8293f52

latest fix #2

Are you sure you want to change the base?

latest fix #2

Uh oh!

Conversation

ozayr-zaviar commented Aug 31, 2020

Summary

Uh oh!

oakbani 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!

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!

oakbani 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!

oakbani 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!

oakbani left a comment

Choose a reason for hiding this comment

Uh oh!

oakbani 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!

Uh oh!