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

Use pytest's monkeypatch.chdir to change the working directory in the config_reader tests #555

Merged
merged 3 commits into from
May 12, 2016

Conversation

ftsamis
Copy link
Member

@ftsamis ftsamis commented May 4, 2016

Following @yeganer's suggestion at #543 (comment) and replacing the custom function in test_config_reader, for temporarily changing the working directory in a test, with pytest's monkeypatch.chdir.

@yeganer
Copy link
Contributor

yeganer commented May 4, 2016

I think you could go even so far as to have one test function and parametrize it. I think you wouldn't need a class in that case.

@wkerzendorf
Copy link
Member

@yeganer @ftsamis are we done here?

@ftsamis
Copy link
Member Author

ftsamis commented May 10, 2016

@yeganer Checked parametrize, and I think it would probably make sense to only use it for 2 of the 3 functions, since the one with the absolute path has slightly different behaviour (no monkeypatch).

If you don't insist on using parametrize, I'd say we can merge this @wkerzendorf.

@wkerzendorf
Copy link
Member

@yeganer @ftsamis looks good - signing off.

@yeganer
Copy link
Contributor

yeganer commented May 10, 2016

Actually I'd use parametrize because the functionality is the same in all 3 functions.

@pytest.parametrize(['.', '..', os.path.abs(os.cwd())])

something like this.
That would make it very obvious what's happening.
@wkerzendorf If you don't share my opinion, you can merge. The rest looks good.

@wkerzendorf
Copy link
Member

@ftsamis I agree with @yeganer. It's a quick change and will reduce the code. Will merge once this is in.

@yeganer yeganer assigned yeganer and wkerzendorf and unassigned yeganer and wkerzendorf May 11, 2016
@wkerzendorf
Copy link
Member

@ftsamis can you fix this?

@ftsamis
Copy link
Member Author

ftsamis commented May 11, 2016

@wkerzendorf I'm now working on it.
@yeganer The first two functions need to change working directory, and to have a relative path. The third doesn't need to change working directory but needs an absolute path. That's the slightly different behaviour I meant.

Anyhow, I'll use parametrize.

@wkerzendorf
Copy link
Member

@yeganer what do you think?

@yeganer
Copy link
Contributor

yeganer commented May 12, 2016

I think, I'm a bit nitpicking here but you could have the cwd as parameter for your function and then read the config two times, once with a relative path to the destination and once with an absolute path. That should cover everything and you don't have to do if/else in the test.

@ftsamis
Copy link
Member Author

ftsamis commented May 12, 2016

That sounds indeed better. Done.

@yeganer
Copy link
Contributor

yeganer commented May 12, 2016

Thanks, that looks great.
@tardis-sn/tardis-core Ready to merge from my side.

@wkerzendorf wkerzendorf merged commit cf8e7a3 into tardis-sn:master May 12, 2016
@ftsamis ftsamis deleted the fix-535-tests branch December 20, 2016 15:29
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