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

[BREAKING] Create a new loop: section and integrate it with the features of the retry: section. #97

Merged
merged 23 commits into from
Aug 20, 2022

Conversation

k1LoW
Copy link
Owner

@k1LoW k1LoW commented Aug 17, 2022

From #90 (comment), I would like to create a new loop: section and integrate it with the features of the retry: section.


steps[*].loop: steps.<key>.loop:

Loop settings for steps.

Simple loop usage

steps:
  multicartin:
    loop: 10
    req:
      /cart/in:
        post:
          body:
            application/json:
              product_id: "{{ i }}" # The loop count (0..9) is assigned to `i`.
[...]

or

steps:
  multicartin:
    loop:
      count: 10
    req:
      /cart/in:
        post:
          body:
            application/json:
              product_id: "{{ i }}" # The loop count (0..9) is assigned to `i`.
[...]

Retry

It can be used as a retry mechanism by setting a condition in the until: section.

If the condition of until: is met, the loop is broken without waiting for the number of count: to be run.

Also, if the run of the number of count: completes but does not satisfy the condition of until:, then the step is considered to be failed.

steps:
  waitingroom:
    loop:
      count: 10
      until: 'steps.waitingroom.res.status == "201"' # Store values of latest loop
      minInterval: 0.5 # sec
      maxInterval: 10  # sec
      # jitter: 0.0
      # interval: 5
      # multiplier: 1.5
    req:
      /cart/in:
        post:
          body:
[...]

( steps[*].retry: steps.<key>.retry: are deprecated )

@k1LoW k1LoW self-assigned this Aug 17, 2022
@@ -355,16 +355,55 @@ steps:
[...]
```

### `steps[*].retry:` `steps.<key>.retry:`
### `steps[*].loop:` `steps.<key>.loop:`
Copy link
Owner Author

Choose a reason for hiding this comment

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

@k2tzumi
To share the implementation image, I updated the README first.

I would like to hear your opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I felt strong feelings 👍

We believe that even though it will be difficult to integrate with the functionality of the retry: section, we will try to make sure that it is not on the same level of hierarchy as the loop: section.

Suggested change
### `steps[*].loop:` `steps.<key>.loop:`
### `steps[*].[req|greq].retry:` `steps.<key>.[req|greq].retry:`

We believe it will be like the above.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I use retry: for both db runner and exec runner.
For example, it can wait when updating values to the database asynchronously.
It is also possible to use it in the include runner to wait.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, there's asynchronous processing.

@k1LoW k1LoW changed the title Create a new loop: section and integrate it with the features of the retry: section. [BREAKING] Create a new loop: section and integrate it with the features of the retry: section. Aug 17, 2022
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@k1LoW k1LoW marked this pull request as ready for review August 19, 2022 08:52
@k1LoW k1LoW added the enhancement New feature or request label Aug 19, 2022
@github-actions

This comment has been minimized.

eval.go Outdated Show resolved Hide resolved
k2tzumi added a commit to k2tzumi/runn that referenced this pull request Aug 20, 2022
@k2tzumi k2tzumi mentioned this pull request Aug 20, 2022
retrySectionKey = "retry"
loopSectionKey = "loop"
deprecatedRetrySectionKey = "retry" // deprecated
loopCountVarKey = "i"
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nits]
How to name the key if we try to implement a counter for the index of steps[*] in a future feature addition? I was wondering.

Is there a way to concisely describe a case that references an index when testing a response in the same step?

steps:
  -
    req:
      /get?var=foo:
        get:
          body: null
    test: |
      steps[0].res.status == 200

I wanted to be concise, e.g., to be able to refer to the index of steps[0] by a variable.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I have the same problem as you (To solve this problem, I created a step listing with a map.

How to name the key if we try to implement a counter for the index of steps[*] in a future feature addition? I was wondering.

I'm certainly wondering what to do about it.

If we were to support both Array and Map types, should there be a way to specify the current step as follows?

steps:
  -
    req:
      /get?var=foo:
        get:
          body: null
    test: |
      current.res.status == 200
steps:
  reqget:
    req:
      /get?var=foo:
        get:
          body: null
    test: |
      current.res.status == 200

Copy link
Owner Author

Choose a reason for hiding this comment

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

If you have any ideas for naming the step[*] counter and the loop counter well, please let me know!

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we were to support both Array and Map types, should there be a way to specify the current step as follows?

The current key looks very nice, since it can be used in either Map or Array.

I'd like to respond separately from this PR.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@k2tzumi Thank you very much!!

Copy link
Owner Author

@k1LoW k1LoW Aug 20, 2022

Choose a reason for hiding this comment

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

@k2tzumi I invited you to become a collaborator of runn!

k1LoW and others added 2 commits August 20, 2022 15:27
Co-authored-by: Katsumi Kato <k2tzumi@users.noreply.github.com>
@github-actions

This comment has been minimized.

operator.go Outdated Show resolved Hide resolved
@k1LoW k1LoW requested a review from k2tzumi August 20, 2022 07:26
Co-authored-by: Katsumi Kato <k2tzumi@users.noreply.github.com>
@github-actions

This comment has been minimized.

@k1LoW k1LoW requested review from k2tzumi and removed request for k2tzumi August 20, 2022 07:30
@k1LoW
Copy link
Owner Author

k1LoW commented Aug 20, 2022

@k2tzumi If you can, I'd be happy to get your comments on whether or not it looks good for us.

@k2tzumi k2tzumi mentioned this pull request Aug 20, 2022
@github-actions

This comment has been minimized.

Comment on lines 86 to 87
steps[5].res.status == 204
# FIXME: I'd like to see the last status be 200.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@k1LoW

What is the best rule for referencing the result of a loop process?

  • Keep only the final result.
    Currently, I think the first result is referenced.
  • Allow reference to the result of each loop.
    steps[5].res[4].status.

Copy link
Owner Author

@k1LoW k1LoW Aug 20, 2022

Choose a reason for hiding this comment

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

Maybe the until: condition is incorrect.

- until: 'steps[5].res.status != "200"'
+ until: 'steps[5].res.status == 200'

Copy link
Owner Author

Choose a reason for hiding this comment

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

Fixed 👍 c26f27c

include:
path: httpbin_include.yml
vars:
jsonRequestBody: 'json://{{ vars.loopVars[i] }}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was expecting the json to be loaded. 😅

I will try to respond as soon as possible.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Great!!!

Copy link
Collaborator

Choose a reason for hiding this comment

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

PR sent.
#101

steps:
-
req:
/post?count={{ vars.counter }}:
Copy link
Collaborator

Choose a reason for hiding this comment

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

[NOTE]
I thought I could reference the loop counter directly in the include destination, but I couldn't.
I don't think it's a problem because there are alternatives as per the test scenario.

Suggested change
/post?count={{ vars.counter }}:
/post?count={{ i }}:

Copy link
Owner Author

@k1LoW k1LoW Aug 20, 2022

Choose a reason for hiding this comment

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

I thought I could reference the loop counter directly in the include destination, but I couldn't.

Yes. Because the position of the i variable is root, I thought it would conflict with the loop counter in the included.

I also thought parent.i would be uncool 😄

@github-actions
Copy link
Contributor

Code Metrics Report

main (3de9ed0) #97 (20472e4) +/-
Coverage 72.2% 72.5% +0.3%
Code to Test Ratio 1:0.7 1:0.7 -0.0
Test Execution Time 1m1s 49s -12s
Details
  |                     | main (3de9ed0) | #97 (20472e4) |  +/-  |
  |---------------------|----------------|---------------|-------|
+ | Coverage            |          72.2% |         72.5% | +0.3% |
  |   Files             |             17 |            17 |     0 |
  |   Lines             |           1886 |          1929 |   +43 |
+ |   Covered           |           1362 |          1399 |   +37 |
- | Code to Test Ratio  |          1:0.7 |         1:0.7 |  -0.0 |
  |   Code              |           3652 |          3723 |   +71 |
+ |   Test              |           2710 |          2712 |    +2 |
+ | Test Execution Time |           1m1s |           49s |  -12s |

Code coverage of files in pull request scope (73.2% → 73.9%)

Files Coverage +/-
cond.go 0.0% -83.3%
eval.go 68.0% +68.0%
loop.go 97.1% +97.1%
operator.go 72.8% -0.2%

Reported by octocov

Copy link
Collaborator

@k2tzumi k2tzumi left a comment

Choose a reason for hiding this comment

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

@k1LoW
LGTM!

@k1LoW
Copy link
Owner Author

k1LoW commented Aug 20, 2022

Thank you!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants