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

Purge RCK test entries in afterEach instead of beforeEach #11699

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

smaheshwar-pltr
Copy link
Contributor

@smaheshwar-pltr smaheshwar-pltr commented Dec 4, 2024

The REST Compatibility Kit (RCK) currently checks whether the test entries used by CatalogTests are present in the Catalog, and doesn't run the tests if so.

This PR, in a similar vein, purges RCK test entries in an afterEach method instead of a beforeEach. I think that this will prevent test entries being potentially left in an actual, tested REST catalog after the RCK finishes, which seems like an improvement. (I'm not too sure though, maybe I'm missing something in which case sorry! :))

@smaheshwar-pltr
Copy link
Contributor Author

cc @danielcweeks maybe, if you have a moment?

@danielcweeks
Copy link
Contributor

@smaheshwar-pltr I think this should be ok since the cleanup should happen regardless of whether the test succeeds or fails. The initial check in the BeforeALL is a safety catch to prevent running against a real catalog and possibly deleting real data. The purge is for cleanup between tests.

@danielcweeks
Copy link
Contributor

@nastra I'm not sure why the checks didn't run here. Are we not setup to run tests if only tests change or is it due to this being part of the open-api project?

@smaheshwar-pltr
Copy link
Contributor Author

smaheshwar-pltr commented Dec 4, 2024

@danielcweeks Thanks a lot for taking a look! As it stands, I agree that clean-up happens correctly between tests (this PR was meant to be a small improvement, not a fix), but with a beforeEach instead of an afterEach for the clean-up, I think that running the RCK on a real catalog may leave spurious tables created by the last RCK test (after which clean up wouldn't occur). Apologies if I'm misunderstanding something, but this sounded slightly undesirable in a similar (but less dire) way that deleting real data does, which maybe prompted the beforeAll check.

But I understand that if this was a safety check, this scenario of running RCK against a real catalog and so this PR might not be meaningful to consider - happy for it to be closed in that case, thanks!

Copy link

github-actions bot commented Jan 4, 2025

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jan 4, 2025
@smaheshwar-pltr
Copy link
Contributor Author

@danielcweeks (very light) ping on this.

My concern is that running RCK on a real catalog may leave spurious tables in the real catalog created by the last RCK test, after which clean up wouldn't occur. This sounds undesirable and is fixed by afterEach clean up.

Happy to close this PR if this is a non-issue.

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

Successfully merging this pull request may close these issues.

2 participants