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

Force skeptic tests to be located in temporary directory #26

Merged
merged 1 commit into from
Jun 13, 2017
Merged

Force skeptic tests to be located in temporary directory #26

merged 1 commit into from
Jun 13, 2017

Conversation

budziq
Copy link
Owner

@budziq budziq commented May 15, 2017

This is a relatively ugly hack reolving https://github.com/brson/rust-skeptic/issues/23

This should solve problem test artifacts polluting working directory such as https://github.com/brson/rust-cookbook/issues/29

@budziq
Copy link
Owner Author

budziq commented Jun 7, 2017

Hi @brson
Is this what you've had in mind in https://github.com/brson/rust-skeptic/issues/23 ?

@brson
Copy link
Collaborator

brson commented Jun 13, 2017

Oh wow! Thanks @budziq. Yeah this is totally the right idea.

This won't quite work correctly as-is because 'set_current_dir` will change the directory for the process globally, and since tests execute concurrently it will cause a race condition.

The way to do this to avoid the race is to pass outdir to the run_test_case function and set current_dir on the Command function that runs the executable. Do you mind trying to make that change and see how it works?

Sorry for not seeing this earlier. Feel free to ping me on IRC or email me if it looks like I'm ignoring your PRs - I mostly don't see GitHub notifications.

@budziq
Copy link
Owner Author

budziq commented Jun 13, 2017

Argh! I haven't really thought this through ☹️
Thanks for your suggestions and time! I'll allot some time this week to fix it.

This should solve problem with files written by test polluting
the working directory.
@budziq
Copy link
Owner Author

budziq commented Jun 13, 2017

@brson updated according to your suggestions. It works well enough to trigger failures in two examples expecting CWD to be rust-cookbook root 😁.

Two points of note:

  • this makes writing tests somewhat less comfortable as skeptic does not provide any way to provide test fixtures (but this can be remedied by hidden test parts written by hand or making them no_run)
  • this is a breaking change which will make some peoples CI fail suddenly so I would suggest bumping to 0.10

Also, I was not able to find a way to write a testcase for this behaviour (running test and then checking for its artifact outside of the runner) that would not require big rewrite.

@brson
Copy link
Collaborator

brson commented Jun 13, 2017

It works well enough to trigger failures in two examples

Haha. I don't mind this. Seems appropriate for those test cases to be independent of the cookbook contents anyway.

this is a breaking change which will make some peoples CI fail suddenly so I would suggest bumping to 0.10

I'll bump the version and make a release.

@brson brson merged commit 33469b1 into budziq:master Jun 13, 2017
@brson
Copy link
Collaborator

brson commented Jun 13, 2017

I published 0.10.0

@budziq
Copy link
Owner Author

budziq commented Jun 13, 2017

Thanks!

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.

2 participants