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

test: add test around the polling server and fix surfaced issues #1112

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

yitsushi
Copy link
Collaborator

@yitsushi yitsushi commented Nov 13, 2023

The test is very simple for now:

  1. Creates some basic test resources.
  2. Uses a fake k8s client.
  3. Uses a fake git provider.
  4. Starts the polling server.
  5. Waits until the branch planner resource appears.
  6. Creates a replan comment.
  7. Waits until the branch planner resource has "reconcile" request
    and placeholder comment id annotation.
  8. Marks the pull request as closed.
  9. Waits until the branch planner resource is deleted.

That covers the basic logic.

Discovered issues:

  1. The isAllowedNamespace check was wrong.
  2. The Source object can have nil Reference for branch.
    I don't know what it does, I assume it uses a default branch, but we
    didn't handle that.

Other changes:

  1. I moved the provider.FromURL into a Server struct level field so we
    can mock the provider layer. I tried to come up other ideas, but that
    looked the less messy.

Closes #837

References:

@yitsushi yitsushi force-pushed the 837-branch-planner-unit-test branch 3 times, most recently from c289070 to e476f14 Compare November 13, 2023 15:33
The test is very simple for now:
1. Creates some basic test resources.
2. Uses a fake k8s client.
3. Uses a fake git provider.
4. Starts the polling server.
5. Waits until the branch planner resource appears.
6. Creates a replan comment.
7. Waits until the branch planner resource has "reconcile" request
   and placeholder comment id annotation.
8. Marks the pull request as closed.
9. Waits until the branch planner resource is deleted.

That covers the basic logic.

Discovered issues:
1. The isAllowedNamespace check was wrong.
2. The Source object can have nil Reference for branch.
   I don't know what it does, I assume it uses a default branch, but we
   didn't handle that.

Other changes:
1. I moved the `provider.FromURL` into a Server struct level field so we
   can mock the provider layer. I tried to come up other ideas, but that
   looked the less messy.

Closes #837

References:
* #837

Signed-off-by: Balazs Nadasdi <[email protected]>
@yitsushi yitsushi force-pushed the 837-branch-planner-unit-test branch from e476f14 to 25a7e91 Compare November 13, 2023 15:33
@yitsushi yitsushi marked this pull request as ready for review November 13, 2023 15:33
Copy link
Contributor

@yiannistri yiannistri left a comment

Choose a reason for hiding this comment

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

Great stuff 👍

@yitsushi yitsushi merged commit 74fd85e into main Nov 14, 2023
15 checks passed
@bigkevmcd bigkevmcd deleted the 837-branch-planner-unit-test branch December 12, 2023 17:58
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.

Write unit tests for the branch planner mechanics
2 participants