Testing phased reviews

[bryan-aguilar]

I am currently evaluating PullApprove for integration into an org I am apart of. During the evaluation process I have ran into a few open questions regarding the testing framework.

Is it possible to test a config file that is using a phased review type structure? My current understanding of the testing framework leads me to believe the initial reviewer approval condition would never be met. I have tested it and only the reviewergroup displays in the test result page with no mention of the approvalgroup.

# approver workflow
version: 3
groups:
  reviewergroup
    conditions:
    - >
      contains_any_fnmatches(files, [
        "files/*",
        ])
    reviewers:
      users:
      - user1
  approvalgroup:
    conditions:
    - "groups.approved == groups.required"
    reviewers:
      users:
      - user2
    reviews:
      required: 1

Q2: does the groups.approved == groups.required logically make sense? If we were to implement pullapprove we would end up with a fairly large number of templated groups to meet our code ownership requirements. I really do not want to write out a large set of conditionals in the approval group structure and am hoping this would fulfill the requirement.

Q3: The groups list contains a passing and approved field. What is the difference in the context of the pull request.

Q4: Manually testing config changes on a custom PR is very cumbersome. Is there any other way to test changes to config files or examples on how this can be integrated into a larger organization?

[davegaeddert]

I'll have to figure out how those definitions got lost in the docs! The short answer is I think you'll want to use len(groups.passing) == len(groups) if you're wanting the last group to be enabled after the others have finished. (It probably works without len too, but I'm not in a place where I can test that right now to say definitively.)

The difference in groups.passing and groups.approved is that groups.passing also includes the groups whose conditions haven't been met (i.e. if you have a "python" group to match .py files and there aren't any in the PR, that group is effectively N/A which is considered "passing").

(groups.required actually refers to the group type. Too many terms floating around especially without the definitions in the docs!)

Did you see the "show inactive groups" toggle? Easy to miss, but that will show the groups where the conditions haven't been met (by default it only shows the ones meeting current conditions - a.k.a. "active" groups). I suspect that's where your approvalgroup was hiding in your test:

And I totally hear you on the testing. One thing to note is that you don't have to test on a custom PR — you can test on any real PR. Just delete the test comments afterwards if you don't want them cluttering up the PR.

It isn't publicly documented yet, but you are welcome to try out the local/CLI testing workflow that is under development. It's a Python package so you can do pip install pullapprove and then there is:

• pullapprove repl <pull_request_url> (for quickly testing one-off expressions)
• pullapprove test --config <test_yaml_filename> <pull_request_url> (similar to the current comment-based testing).

Let me know what questions you have. Hope that helps at least a little bit!

[bryan-aguilar]

Thanks for the response Dave. I did not notice the show inactive groups toggle and I will check that out.

Thanks for the explanation on the groups field, there were quite a few that sounded similar without clarification in the docs.

I will also look into the command line testing to see how it can supplement our proposed workflow. In a perfect world it would be great to have some type of unit testable functionality. As I was poking around the tool I really wanted a way to mock out PR states so I could make sure my all my conditional logic makes sense. In the phased review example I would want to make sure that my approval group is triggered if previous groups were in a passing state. This is definitely a big feature ask but that's what I was imagining in my head as I was using PullApprove.

Again, I appreciate you getting back so quickly and I will let you know if any other open questions arise in the coming week.

[davegaeddert]

Appreciate the feedback. Getting it off the ground is definitely the hardest part right now. I think people/orgs find the iterative changes afterwards to be a lot easier, as you'd probably expect. Same challenges as a lot of the CI service configs etc... feels like the local / unit testing is at least part of the answer! Can't make any promises on the "testing framework" sort of idea but we're probably getting closer to that being a possibility. Could maybe do something where you mock partial fields for the PR / GitHub API:

pull_request:
  # https://docs.github.com/en/rest/reference/pulls#list-pull-requests-files
  files:
  - filename: test.py

  # https://docs.github.com/en/rest/reference/pulls#list-reviews-for-a-pull-request
  reviews:
  - user:
      login: user1
    state: APPROVED

expect:
  state: pending
  groups:
    reviewergroup:
      state: approved
    approvalgroup:
      state: pending

Hard to say how much use it would get, but it might be worth trying at some point.

[bryan-aguilar]

Just getting the ability to mock PR/reviewer state would be very helpful. Thanks!