Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FFM-5307] - .NET SDK - Fix for prereq identifier + update ff-test-cases #47

Merged
merged 1 commit into from Nov 29, 2022

Conversation

andybharness
Copy link
Contributor

@andybharness andybharness commented Nov 28, 2022

FFM-5307 - .NET SDK - Fix for prereq identifier + update JSON parse to use latest tests

What
When a feature depends on another prereq feature being true, the evaluation fails if the value and identifier are different values in the prereq. Update code to use latest ff-test-cases JSON format which adds ~90 additional tests. Some additional fixes were needed including Null reference exception and incorrect OR handling in groups when latest JSON was used.

Why
The checkPreRequisite() method incorrectly uses the value of the prereq instead of the identifier.

Testing
Tested locally with new ff-test-cases JSON + sanity checks with example program

…r to use latest tests

What
When a feature depends on another prereq feature being true, the evaluation fails if the value and identifier are different values in the prereq. Update code to use latest ff-test-cases JSON format.
Some additional fixes were needed including Null reference exception and incorrect OR handling in groups when latest JSON was used.

Why
The checkPreRequisite() method incorrectly uses the value of the prereq instead of the identifier.

Testing
Tested locally with new ff-test-cases JSON + sanity checks with example program
Copy link
Contributor

@erdirowlands erdirowlands left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@erdirowlands erdirowlands left a comment

Choose a reason for hiding this comment

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

LGTM (accidentally ticked request changes last time)

Copy link
Collaborator

@davejohnston davejohnston left a comment

Choose a reason for hiding this comment

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

LGTM and the tests pass too !

@andybharness andybharness merged commit 11eca6f into main Nov 29, 2022
@bmjen bmjen deleted the FFM-5307-fix-prereq-and-ff-test-cases branch November 29, 2022 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants