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

Support multi-values (expect_one_of) in jsonpath tolerance expect #191

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

saravanan30erd
Copy link

@saravanan30erd saravanan30erd commented Oct 17, 2020

Added support for multi-values (expect_one_of) in jsonpath tolerance expect field.

Example:

"tolerance": {
                    "type": "jsonpath",
                    "path": "$.status",
                    "expect": ["yellow"],
                    "target": "body"
                },

In this case, path $.status provides value either ["green"] or ["yellow"] or ["red"]. Both yellow and green considered as healthy status. Example: In Elastic-search cluster, If you remove or shutdown one or two nodes in cluster, the status will be green before action and yellow after action. But both should be considered as healthy state in ES.

Currently it doesn't support or / expect_one_of feature in expect value.

so added support for expect_one_of feature in expect value.

  "tolerance": {
                    "type": "jsonpath",
                    "path": "$.status",
                    "expect": [["yellow", "green"]],
                    "target": "body"
                },

If we want to validate the output against multiple expected values, then we can use list of values instead of string.

Output value: ["green", "completed"] OR sometimes  ["yellow", "completed"]

**Default**:
Expect:  ["green", "completed"]
**expect_one_of**
Expect:  [["green", "yellow"], "completed"]

@saravanan30erd saravanan30erd force-pushed the expect_one_of branch 2 times, most recently from ad70586 to 2e4ef91 Compare October 17, 2020 16:57
@saravanan30erd
Copy link
Author

@Lawouach I did a mistake, thought to use list inside list to mention both expect_one_of values bcoz I used one sample json payload which is straight forward. But later realized this won't work for other payloads so tried some other method but it didn't suit for some complex payloads.

At last, I used another expect field called expect_alt to mention the alternative expected values.

Output value: ["green", "completed"] OR sometimes  ["yellow", "completed"]

Default:
Expect:  ["green", "completed"]

expect_one_of:
Expect:  ["green", "completed"],
Expect_alt:  ["yellow", "completed"],

@Lawouach
Copy link
Contributor

Hello, for a better review, could you please squahs your commits and force push?

Copy link
Contributor

@Lawouach Lawouach left a comment

Choose a reason for hiding this comment

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

Hey @saravanan30erd I appreciate the effort and thank you for the tests but I think this needs a bit more work.

The naming expect_alt is confusing to me, I don't understand what it does from its name.

Side note, could you add a changelog entry too? Also there will be a need for a spec PR too (on chaostoolkit-documentation). Maybe we should start there to clarify the goal?

@saravanan30erd
Copy link
Author

@Lawouach

expect_alt- its alternative expect field for expect.
since we are adding one more expect field for using alternative values, I named it as expect_alt
Please provide your suggestion for naming convention. I will use that name.

Sure, I will raise the PR for documentation once the naming convention is finalized.

@Lawouach
Copy link
Contributor

I thought expect_one_of was more direct and descriptive. Unless expecte_alt is behaving differentlty, I'd go for expect_one_of.

@saravanan30erd
Copy link
Author

@Lawouach Renamed the new field as expect_one_of and also added change log.

@codecov-io
Copy link

codecov-io commented Oct 21, 2020

Codecov Report

Merging #191 into master will decrease coverage by 0.04%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
- Coverage   88.02%   87.97%   -0.05%     
==========================================
  Files          25       25              
  Lines        2113     2121       +8     
==========================================
+ Hits         1860     1866       +6     
- Misses        253      255       +2     
Impacted Files Coverage Δ
chaoslib/hypothesis.py 89.37% <77.77%> (-0.58%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f781dfe...e196962. Read the comment docs.

@Lawouach Lawouach marked this pull request as draft February 5, 2021 12:01
Signed-off-by: saravanan palanisamy <[email protected]>

add debug log - jsonpath tolerance

Signed-off-by: saravanan palanisamy <[email protected]>

add new field for expect alternative values

Signed-off-by: saravanan palanisamy <[email protected]>

add debug log for expect_alt

Signed-off-by: saravanan palanisamy <[email protected]>

add test cases for expect_alt

Signed-off-by: saravanan palanisamy <[email protected]>

rename the second expect field & add changelog

Signed-off-by: saravanan palanisamy <[email protected]>
@saravanan30erd saravanan30erd marked this pull request as ready for review March 4, 2021 13:12
@saravanan30erd saravanan30erd requested a review from Lawouach March 4, 2021 13:13
Copy link
Contributor

@Lawouach Lawouach left a comment

Choose a reason for hiding this comment

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

LGTM

But needs a couple of minor changes.

We'll also need to update the specification at https://docs.chaostoolkit.org/reference/api/experiment/#steady-state-probe-tolerance

chaoslib/hypothesis.py Outdated Show resolved Hide resolved
chaoslib/hypothesis.py Outdated Show resolved Hide resolved
@saravanan30erd saravanan30erd force-pushed the expect_one_of branch 4 times, most recently from c1dbff4 to 3976645 Compare March 9, 2021 15:01
Signed-off-by: saravanan palanisamy <[email protected]>

changes based on feedback

Signed-off-by: saravanan palanisamy <[email protected]>

changes based on feedback

Signed-off-by: saravanan palanisamy <[email protected]>
@saravanan30erd
Copy link
Author

@Lawouach Did changes based on your comments. Also added the information in documentation and raised PR.
chaostoolkit/chaostoolkit-documentation#103

@saravanan30erd saravanan30erd requested a review from Lawouach March 9, 2021 17:50
@Lawouach Lawouach marked this pull request as draft March 25, 2021 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants