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: Integration test admin delete #194

Merged
merged 1 commit into from
May 17, 2024

Conversation

KIrie-0217
Copy link
Contributor

Issue #, if available:
#143

Description of changes:
I have added the test for dy admin delete command.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@KIrie-0217
Copy link
Contributor Author

KIrie-0217 commented Dec 6, 2023

During the execution of the destructor for TestManager tm, cleanup is performed on a table that has already been deleted, which causes an error.
To avoid this, the destructor's processing is skipped using std::mem::forget.
While this is not unsafe, the use of std::mem::forget may potentially lead to memory leaks in the future.

I would like you to evaluate whether this implementation is valid or not.

https://github.com/KIrie-0217/dynein/blob/integration-test-admin-delete/tests/admin_delete.rs#L64-L65

@StoneDot StoneDot added the test Something related to test label Mar 1, 2024
@KIrie-0217 KIrie-0217 requested a review from a team as a code owner May 14, 2024 14:19
Copy link
Contributor

@StoneDot StoneDot left a comment

Choose a reason for hiding this comment

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

Thank you for adding tests for dy admin delete command. I left some comments. Could you please check them?

tests/util/mod.rs Outdated Show resolved Hide resolved
tests/util/mod.rs Outdated Show resolved Hide resolved
tests/util/mod.rs Outdated Show resolved Hide resolved
StoneDot
StoneDot previously approved these changes May 15, 2024
Copy link
Contributor

@StoneDot StoneDot left a comment

Choose a reason for hiding this comment

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

Thank you for polishing up the code. Now, LGTM. I left some comments for additional improvement. If you'd like to include them in this pull request, I'll check them after you change them.

tests/util/mod.rs Outdated Show resolved Hide resolved
tests/admin_delete.rs Show resolved Hide resolved
Copy link
Contributor

@StoneDot StoneDot left a comment

Choose a reason for hiding this comment

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

I think added logic is reasonable. Could you remove the unused function count_temporary_tables?

tests/util/mod.rs Outdated Show resolved Hide resolved
StoneDot
StoneDot previously approved these changes May 15, 2024
Copy link
Contributor

@StoneDot StoneDot left a comment

Choose a reason for hiding this comment

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

Thank you for improving the pull request. All looks great to me.

@KIrie-0217 KIrie-0217 force-pushed the integration-test-admin-delete branch from 165ed2e to 7b63afd Compare May 15, 2024 15:36
@KIrie-0217 KIrie-0217 requested a review from a team as a code owner May 15, 2024 15:36
Added method on TestManager to remove a temporary table to prevent double deletion in Drop trait

Signed-off-by: KIrie-0217 <kook.cuckoo@gmail.com>
@StoneDot StoneDot merged commit b526593 into awslabs:main May 17, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Something related to test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants