Fix/client spec constraint tests #116

gardleopard · 2022-09-08T13:26:23Z

This makes us pass the test cases for constraints in:
https://github.com/Unleash/client-specification/blob/main/specifications/13-constraint-operators.json

This makes us ignore the constraint if the constraint operator is unknown.

If input data to a constraint is missing, then its always not present in a set. Nothing is not a part of anything.

lib/unleash/constraint.rb

sighphyre

This looks good to me but give me a bit of time to double check the client spec tests, this got crazy and I want to make sure we aren't crazy too

sighphyre · 2022-09-08T13:35:22Z

Okay, never mind, looked over the client spec, this does look correct and the failing client spec test cases are because... well... nothing can't be in anything - i.e. this is isolated to the oddities of the IN/NOT_IN operators

The remaining failures are due to global constraints not being implemented and should be closed by #114

…invalid

coveralls · 2022-09-09T07:23:48Z

Pull Request Test Coverage Report for Build 3020655652

-1 of 22 (95.45%) changed or added relevant lines in 2 files are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.01%) to 91.102%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/unleash/constraint.rb	11	12	91.67%

Files with Coverage Reduction	New Missed Lines	%
lib/unleash/constraint.rb	3	80.0%

Totals
Change from base Build 3014380490:	0.01%
Covered Lines:	2017
Relevant Lines:	2214

💛 - Coveralls

coveralls · 2022-09-09T07:23:49Z

Pull Request Test Coverage Report for Build 3036905670

36 of 36 (100.0%) changed or added relevant lines in 4 files are covered.
5 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-91.09%) to 0.0%

Files with Coverage Reduction	New Missed Lines	%
lib/unleash/constraint.rb	5	78.95%

Totals
Change from base Build 3014380490:	-91.09%
Covered Lines:	0
Relevant Lines:	2092

💛 - Coveralls

sighphyre

Still looks good but can we make Rubocop happy

rarruda

left some suggestions.

but as @sighphyre commented, rubocop doesn't seem quite happy yet.

lib/unleash/constraint.rb

spec/unleash/constraint_spec.rb

lib/unleash/constraint.rb

spec/unleash/constraint_spec.rb

Co-authored-by: Renato Arruda <rarruda@rarruda.org>

lib/unleash/constraint.rb

Co-authored-by: Renato Arruda <rarruda@rarruda.org>

rarruda

I think I found a corner case that needs to be looked at, plus some other general comments.

rarruda · 2022-09-12T05:17:58Z

lib/unleash/constraint.rb

+      # when the operator is NOT_IN and there is no data, return true. In all other cases the operator doesn't match.
+      return true if operator == :NOT_IN


Think about it again, this seems to belong in matches_constraint? Method and not in the matches_context?

So for this operator the matches_constraint? Should not throw an exception. (But still should for all other operators).

Also note that this doesn't seem to respect the inverted method parameter. Noting it up the stack should solve that. But maybe add a trivial assert for NOT using the inverted parameter (in addition to the current one using)?

The latest commit should solve all of this. The reversing happens in matches_context and in matches_constraint we are now handling the nil values.

spec/unleash/constraint_spec.rb

lib/unleash/constraint.rb

Co-authored-by: Renato Arruda <rarruda@rarruda.org>

rarruda

some more feedback. It is looking promising!

lib/unleash/context.rb

spec/unleash/constraint_spec.rb

lib/unleash/constraint.rb

rarruda · 2022-09-13T09:39:27Z

lib/unleash/constraint.rb

+      # when the operator is NOT_IN and there is no data, return true. In all other cases the operator doesn't match.
+      return self.operator == :NOT_IN unless context.property?(self.context_name)
+
+      Unleash.logger.debug "Unleash::Constraint matches_context? value: #{self.value} context.get_by_name(#{self.context_name})" \
+      " #{context.get_by_name(self.context_name)} "


should switch order here, and rename the method name within the log message / rewrite it to the log messages current context.

Suggested change

# when the operator is NOT_IN and there is no data, return true. In all other cases the operator doesn't match.

return self.operator == :NOT_IN unless context.property?(self.context_name)

Unleash.logger.debug "Unleash::Constraint matches_context? value: #{self.value} context.get_by_name(#{self.context_name})" \

" #{context.get_by_name(self.context_name)} "

Unleash.logger.debug "Unleash::Constraint matches_constraint? value: #{self.value} constraint: #{self.operator} context.get_by_name(#{self.context_name})" \

" #{context.get_by_name(self.context_name)} "

# when the operator is NOT_IN and there is no data, return true. In all other cases the operator doesn't match.

return self.operator == :NOT_IN unless context.property?(self.context_name)

We cant log it before we validate that it is set. This is the reason the log is behind the check if property is set.

why not? context.get_by_name() should return nil if no value is found.

We would like to get this debug message for all operators, including the :NOT_IN, and even if the value is not in the context.

Also the node regarding having matches_constraint? instead of matches_context? within the log message is still valid.

rarruda · 2022-09-13T09:42:07Z

spec/unleash/constraint_spec.rb

-          Unleash::Constraint.new('env', operator_name, '')
-        end.to raise_error
+        expect(Unleash.logger).to receive(:warn).with("value is a String, operator is expecting an Array")
+        Unleash::Constraint.new('env', operator_name, '')
      end

      string_constraints = ['NUM_EQ', 'NUM_GT', 'NUM_GTE', 'NUM_LT', 'NUM_LTE',
                            'DATE_AFTER', 'DATE_BEFORE', 'SEMVER_EQ', 'SEMVER_GT', 'SEMVER_LT']
      string_constraints.each do |operator_name|
        Unleash::Constraint.new('env', operator_name, '')


No need to test that the creation of the object succeeded w/o an exception anymore. It now should be a safe assumption.

Suggested change

Unleash::Constraint.new('env', operator_name, '')

I added this test based on review you had earlier. Its not testing the object creation, its checking that unleash logs a warn message.

We have two Unleash::Constraint.new in this test (per operator_name). We only need to test the "sad" path, that the logger is getting called. No need to test the happy path....

That is at least what I meant to say.

rarruda · 2022-09-13T09:42:25Z

spec/unleash/constraint_spec.rb

+      expect(constraint.matches_context?(context)).to be true
+    end
+
+    it 'warns about constraint construction for invalid value types for operator' do
      array_constraints = ['STR_CONTAINS', 'STR_ENDS_WITH', 'STR_STARTS_WITH', 'IN', 'NOT_IN']

      array_constraints.each do |operator_name|
        Unleash::Constraint.new('env', operator_name, [])


No need to test that the creation of the object succeeded w/o an exception anymore. It now should be a safe assumption.

Suggested change

Unleash::Constraint.new('env', operator_name, [])

I added this test based on review you had earlier. Its not testing the object creation, its checking that unleash logs a warn message.

We have two Unleash::Constraint.new in this test (per operator_name). We only need to test the "sad" path, that the logger is getting called. No need to test the happy path....

That is at least what I meant to say.

spec/unleash/context_spec.rb

Co-authored-by: Renato Arruda <rarruda@rarruda.org>

This reverts commit a8c65ac.

rarruda · 2022-09-13T10:24:21Z

spec/unleash/constraint_spec.rb

+      expect(constraint.matches_context?(context)).to be true
+    end
+
+    it 'warns about constraint construction for invalid value types for operator' do
      array_constraints = ['STR_CONTAINS', 'STR_ENDS_WITH', 'STR_STARTS_WITH', 'IN', 'NOT_IN']

      array_constraints.each do |operator_name|
        Unleash::Constraint.new('env', operator_name, [])


We have two Unleash::Constraint.new in this test (per operator_name). We only need to test the "sad" path, that the logger is getting called. No need to test the happy path....

That is at least what I meant to say.

rarruda · 2022-09-13T10:24:25Z

spec/unleash/constraint_spec.rb

-          Unleash::Constraint.new('env', operator_name, '')
-        end.to raise_error
+        expect(Unleash.logger).to receive(:warn).with("value is a String, operator is expecting an Array")
+        Unleash::Constraint.new('env', operator_name, '')
      end

      string_constraints = ['NUM_EQ', 'NUM_GT', 'NUM_GTE', 'NUM_LT', 'NUM_LTE',
                            'DATE_AFTER', 'DATE_BEFORE', 'SEMVER_EQ', 'SEMVER_GT', 'SEMVER_LT']
      string_constraints.each do |operator_name|
        Unleash::Constraint.new('env', operator_name, '')


We have two Unleash::Constraint.new in this test (per operator_name). We only need to test the "sad" path, that the logger is getting called. No need to test the happy path....

That is at least what I meant to say.

lib/unleash/constraint.rb

rarruda · 2022-09-13T10:31:39Z

lib/unleash/constraint.rb

+      # when the operator is NOT_IN and there is no data, return true. In all other cases the operator doesn't match.
+      return self.operator == :NOT_IN unless context.property?(self.context_name)
+
+      Unleash.logger.debug "Unleash::Constraint matches_context? value: #{self.value} context.get_by_name(#{self.context_name})" \
+      " #{context.get_by_name(self.context_name)} "


why not? context.get_by_name() should return nil if no value is found.

We would like to get this debug message for all operators, including the :NOT_IN, and even if the value is not in the context.

Also the node regarding having matches_constraint? instead of matches_context? within the log message is still valid.

This reverts commit 82bb576.

sighphyre

Woot!

gardleopard added 2 commits September 8, 2022 13:19

fix: constraint test if operator is unknown

3b0eef6

This makes us ignore the constraint if the constraint operator is unknown.

fix: nothing can't be in anything

5821459

If input data to a constraint is missing, then its always not present in a set. Nothing is not a part of anything.

sonatype-lift bot reviewed Sep 8, 2022

View reviewed changes

lib/unleash/constraint.rb Outdated Show resolved Hide resolved

sonatype-lift bot reviewed Sep 8, 2022

View reviewed changes

lib/unleash/constraint.rb Outdated Show resolved Hide resolved

sighphyre reviewed Sep 8, 2022

View reviewed changes

linting

71a18ce

gardleopard added 3 commits September 9, 2022 08:34

refactor: remove not_in tests from in test

a8c65ac

fix: make unleash not rejecting configuration of constraints that is …

b6b6f17

…invalid

cleaner log messages

154392a

sighphyre requested changes Sep 9, 2022

View reviewed changes

rarruda reviewed Sep 9, 2022

View reviewed changes

gardleopard and others added 11 commits September 9, 2022 13:46

Update lib/unleash/constraint.rb

67d7dfd

Co-authored-by: Renato Arruda <rarruda@rarruda.org>

Update spec/unleash/constraint_spec.rb

8d19450

Co-authored-by: Renato Arruda <rarruda@rarruda.org>

refactor

5adf0c5

linting

a79c144

linting

8cf2978

linting

fcfbce6

linting

b269699

linting

0ef3384

linting

ecc88b0

linting

36bd020

fix

0662bb0

rarruda reviewed Sep 9, 2022

View reviewed changes

lib/unleash/constraint.rb Outdated Show resolved Hide resolved

rarruda reviewed Sep 9, 2022

View reviewed changes

lib/unleash/constraint.rb Outdated Show resolved Hide resolved

gardleopard and others added 3 commits September 9, 2022 15:20

Update lib/unleash/constraint.rb

3564be1

Co-authored-by: Renato Arruda <rarruda@rarruda.org>

Update lib/unleash/constraint.rb

d4ef1b0

Co-authored-by: Renato Arruda <rarruda@rarruda.org>

change loglevel to warn and have test for logging it

7f54e74

rarruda reviewed Sep 12, 2022

View reviewed changes

gardleopard and others added 4 commits September 12, 2022 08:29

Update spec/unleash/constraint_spec.rb

6a418d2

Co-authored-by: Renato Arruda <rarruda@rarruda.org>

refactor

3550faf

move logic to evaluation

9e43d83

make rubocop happy

1ed8b88

rarruda reviewed Sep 13, 2022

View reviewed changes

gardleopard and others added 4 commits September 13, 2022 11:51

Update spec/unleash/context_spec.rb

6460faa

Co-authored-by: Renato Arruda <rarruda@rarruda.org>

Revert "refactor: remove not_in tests from in test"

554fa1c

This reverts commit a8c65ac.

s/property?/include?/g

3dbe698

include the inverted test for invalid operators

e408f69

rarruda reviewed Sep 13, 2022

View reviewed changes

gardleopard and others added 6 commits September 13, 2022 12:40

dont test the happy path

575d95f

refactor

6fa901a

fix

1286d83

fix log message linebreak

82bb576

fix log messages linebreak

56ddbaa

Revert "fix log message linebreak"

2adc1ac

This reverts commit 82bb576.

rarruda approved these changes Sep 13, 2022

View reviewed changes

rarruda requested a review from sighphyre September 13, 2022 11:11

sighphyre approved these changes Sep 13, 2022

View reviewed changes

gardleopard merged commit 285ab7e into main Sep 13, 2022

gardleopard deleted the fix/client_spec_constraint_tests branch September 13, 2022 11:40

rarruda mentioned this pull request Sep 13, 2022

fix: add test case for missing context field #110

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/client spec constraint tests #116

Fix/client spec constraint tests #116

gardleopard commented Sep 8, 2022

sighphyre left a comment

sighphyre commented Sep 8, 2022

coveralls commented Sep 9, 2022

coveralls commented Sep 9, 2022 •

edited

Loading

sighphyre left a comment •

edited

Loading

rarruda left a comment

rarruda left a comment

rarruda Sep 12, 2022

gardleopard Sep 12, 2022

rarruda left a comment

rarruda Sep 13, 2022

gardleopard Sep 13, 2022

rarruda Sep 13, 2022

rarruda Sep 13, 2022

gardleopard Sep 13, 2022

rarruda Sep 13, 2022

rarruda Sep 13, 2022

gardleopard Sep 13, 2022

rarruda Sep 13, 2022

gardleopard Sep 13, 2022

rarruda Sep 13, 2022

rarruda Sep 13, 2022

rarruda Sep 13, 2022

sighphyre left a comment

		# when the operator is NOT_IN and there is no data, return true. In all other cases the operator doesn't match.
		return true if operator == :NOT_IN

Fix/client spec constraint tests #116

Fix/client spec constraint tests #116

Conversation

gardleopard commented Sep 8, 2022

sighphyre left a comment

Choose a reason for hiding this comment

sighphyre commented Sep 8, 2022

coveralls commented Sep 9, 2022

Pull Request Test Coverage Report for Build 3020655652

💛 - Coveralls

coveralls commented Sep 9, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3036905670

💛 - Coveralls

sighphyre left a comment • edited Loading

Choose a reason for hiding this comment

rarruda left a comment

Choose a reason for hiding this comment

rarruda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rarruda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sighphyre left a comment

Choose a reason for hiding this comment

coveralls commented Sep 9, 2022 •

edited

Loading

sighphyre left a comment •

edited

Loading