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: update testing suite to write to a more visible path #21

Merged
merged 2 commits into from
Jul 15, 2022

Conversation

tylerslaton
Copy link
Collaborator

Summary

With this fix the testing suite now writes to ./test_runs which is a local directory that git ignores. This should make these runs more visible to easily delete.

@tylerslaton tylerslaton changed the title test: update testing suite to write to a more visible path for easy deletion test: update testing suite to write to a more visible path Jul 15, 2022
@tylerslaton tylerslaton force-pushed the update-test-location branch from 9d29330 to 49fc858 Compare July 15, 2022 13:38
@tylerslaton tylerslaton requested review from exdx and timflannagan July 15, 2022 13:39
Copy link
Owner

@exdx exdx left a comment

Choose a reason for hiding this comment

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

Looks good. Can we go ahead and add cleanup for the directory while we're making this change?

@tylerslaton tylerslaton force-pushed the update-test-location branch from 8a17940 to 731ce96 Compare July 15, 2022 14:03
Copy link
Collaborator

@timflannagan timflannagan left a comment

Choose a reason for hiding this comment

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

Any idea on what happens if the test case assertion fails? Does it drop down to the remove_dir_all function, or does the function flow immediately stop there?

@tylerslaton tylerslaton force-pushed the update-test-location branch from 731ce96 to 3bdddbf Compare July 15, 2022 14:05
Copy link
Owner

@exdx exdx left a comment

Choose a reason for hiding this comment

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

LGTM

tests/cli.rs Outdated Show resolved Hide resolved
@exdx
Copy link
Owner

exdx commented Jul 15, 2022

Any idea on what happens if the test case assertion fails? Does it drop down to the remove_dir_all function, or does the function flow immediately stop there?

Yes, that's a good call-out. We need the equivalent of a defer() or AfterEach() to handle that case

@tylerslaton
Copy link
Collaborator Author

Any idea on what happens if the test case assertion fails? Does it drop down to the remove_dir_all function, or does the function flow immediately stop there?

I would think that it would fail to clean-up the directory so thats a good callout. Let me try and find some sort of defer in rust.

@exdx
Copy link
Owner

exdx commented Jul 15, 2022

There's some workaround using the Drop trait, but it seems complex.

@tylerslaton tylerslaton force-pushed the update-test-location branch from 3bdddbf to 69dcb36 Compare July 15, 2022 14:52
@tylerslaton tylerslaton merged commit 664d0c4 into exdx:main Jul 15, 2022
@tylerslaton tylerslaton deleted the update-test-location branch July 15, 2022 15:01
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.

3 participants