-
Notifications
You must be signed in to change notification settings - Fork 3
Refactor tests to remove the embedded git #32
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of niggles, otherwise it looks like a big improvement.
I've thought of one possible improvement, but this is already a big enough PR so it could be done separately. Rather than use the hardcoded path Line 7 in 94b9870
Then each test will definitely be isolated from the other tests (each will have its own working_dir ), and we won't have to cleanup after each test (e.g. removing the cloned xtl repo) as the temporary directories will automatically be cleaned for us.
Also I will create an issue for checking the |
test/test_branch.py
Outdated
p = subprocess.run(cmd, capture_output=True, cwd="test/data/status_data", text=True) | ||
assert(p.stdout == '* main\n') | ||
p = subprocess.run(cmd, capture_output=True, cwd=working_dir, text=True) | ||
print() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, empty print
statement. I assume it isn't needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always forget a print
left in my code somewhere...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least there were no swear words in it!
I'll change to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @SandrineP
Refactor tests to remove the embedded git