Skip to content

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

Merged
merged 12 commits into from
Aug 7, 2025

Conversation

SandrineP
Copy link
Collaborator

Refactor tests to remove the embedded git

@SandrineP SandrineP marked this pull request as draft August 4, 2025 09:33
@SandrineP SandrineP marked this pull request as ready for review August 7, 2025 09:29
Copy link
Member

@ianthomas23 ianthomas23 left a 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.

@ianthomas23
Copy link
Member

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 test/data for the tests, we could instead use the pytest fixture tmp_path like we do at

def run_in_tmp_path(tmp_path):

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 returncode after each subprocess.run call in the tests.

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()
Copy link
Member

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?

Copy link
Collaborator Author

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...

Copy link
Member

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!

@SandrineP
Copy link
Collaborator Author

I'll change to tmp_path in another PR as suggested.

Copy link
Member

@ianthomas23 ianthomas23 left a comment

Choose a reason for hiding this comment

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

Thanks @SandrineP

@ianthomas23 ianthomas23 merged commit cca4403 into QuantStack:main Aug 7, 2025
1 check passed
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