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

Add TestAssert support for errors files #457

Closed
wants to merge 4 commits into from

Conversation

iblancasa
Copy link
Collaborator

Signed-off-by: Israel Blancas [email protected]

Fixes #456

@erikgb
Copy link
Contributor

erikgb commented Aug 30, 2023

I am not sure if I fully understand the motivation for this PR. Will a TestAssert in an errors-file mean something else than if you put it in an assert-file? If the answer is no, I don't see that we need this. Why not just put the TestAssert in an otherwise empty assert-file?

@iblancasa
Copy link
Collaborator Author

I am not sure if I fully understand the motivation for this PR. Will a TestAssert in an errors-file mean something else than if you put it in an assert-file? If the answer is no, I don't see that we need this. Why not just put the TestAssert in an otherwise empty assert-file?

The fix provided as part of this PR was motivated by this thread in Slack.

Somebody tried to do this in a x-errors.yaml file:

apiVersion: kuttl.dev/v1beta1
kind: TestAssert
timeout: 300
---
apiVersion: v1
kind: Service
metadata:
  name: gitea-http
  namespace: gitea
---
apiVersion: v1
kind: Pod
metadata:
  name: gitea-0
  namespace: gitea
---

And kuttl fails with the following error:

    case.go:364: failed in step 0-delete
    case.go:366: retrieving API resource for kuttl.dev/v1beta1, Kind=TestAssert failed: the server could not find the requested resource

So, instead of interpreting the TestAssert object as a command, kuttl is trying to find that resource in the server.

@erikgb
Copy link
Contributor

erikgb commented Aug 30, 2023

Thanks for the pointer, I remember the thread now. I personally think a TestAssert "resource" belongs in an assert-file. Does my suggestion above (an assert-file with a single TestAssert) solve the use-case?

Copy link
Member

@porridge porridge left a comment

Choose a reason for hiding this comment

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

I agree with @erikgb that this is not strictly necessary, but it's a papercut that can be easily avoided. I like this change because it brings about nice symmetry to the assert/error behaviour. I.e. now they are the same, except for the negation.

Thanks for also fixing the silent ignoring of all but last encountered TestAssert in a step. Please mention this in the PR description and title so it shows up in the release notes, as technically it's an incompatible change (some suites which contained superfluous TestAssert obejcts that previously passed will now abort).

@iblancasa
Copy link
Collaborator Author

Thanks for the pointer, I remember the thread now. I personally think a TestAssert "resource" belongs in an assert-file. Does my suggestion above (an assert-file with a single TestAssert) solve the use-case?

It can be solved/workarounded (I don't know if that word is correct). But it is not obvious if you are not too much in kuttl. If it helps people to use kuttl and doesn't break anything, I think it is a good change :D (but I don't have a super strong opinion about adding it).

Copy link
Contributor

@erikgb erikgb left a comment

Choose a reason for hiding this comment

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

Thanks for elaborating. I think this is good - at least for now.

@iblancasa iblancasa closed this Apr 8, 2024
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.

Support TestAssert in errors steps
3 participants