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

Egork/vila unitet test to class #169

Merged
merged 29 commits into from
Oct 22, 2022
Merged

Conversation

egork520
Copy link
Contributor

Moving unite tests to the class. Locally pytest test would fail on this file

@soldni
Copy link
Member

soldni commented Oct 20, 2022

Hey @comorado, I am not sure if it recommended to move directories during testing: I can imagine introducing a bug during tests that is caused by a previous test moving to a different directory.

In tests I've authored, I use __file__ to get an absolute path to fixtures, e.g. see tests/test_parsers/test_override.py.

Some StackOverflow answers recommend creating functions pointing to paths and decorating them with @pytest.fixture

@egork520
Copy link
Contributor Author

Hey @comorado, I am not sure if it recommended to move directories during testing: I can imagine introducing a bug during tests that is caused by a previous test moving to a different directory.

Good point @soldni, that's the reason why I had it in setUp method. setUp is run prior to running each individual test https://stackoverflow.com/questions/6854658/explain-the-setup-and-teardown-python-methods-used-in-test-cases

In tests I've authored, I use __file__ to get an absolute path to fixtures, e.g. see tests/test_parsers/test_override.py.
I like your suggestion and instead of changing the directories I will create a class variable which points to the fixtures path.

Some StackOverflow answers recommend creating functions pointing to paths and decorating them with @pytest.fixture

A bit too fancy, thank you for the pointer :)

@egork520
Copy link
Contributor Author

Hey @comorado, I am not sure if it recommended to move directories during testing: I can imagine introducing a bug during tests that is caused by a previous test moving to a different directory.

Good point @soldni, that's the reason why I had it in setUp method. setUp is run prior to running each individual test https://stackoverflow.com/questions/6854658/explain-the-setup-and-teardown-python-methods-used-in-test-cases

In tests I've authored, I use __file__ to get an absolute path to fixtures, e.g. see tests/test_parsers/test_override.py.
I like your suggestion and instead of changing the directories I will create a class variable which points to the fixtures path.
Some StackOverflow answers recommend creating functions pointing to paths and decorating them with @pytest.fixture

A bit too fancy, thank you for the pointer :)

@soldni I've pushed a commit where I am creating a path variable. Still doing it though in the setUp method. On the pytest run time each individual test receives it's version of the setUp variables. So for example if self.doc was modified in one test, another test is not going to see the modifications because it receives its version of the self.doc created by the setUp class.

@egork520 egork520 requested a review from soldni October 20, 2022 19:44
@egork520 egork520 force-pushed the egork/vila-unitet-test-to-class branch from bfa5108 to a425583 Compare October 20, 2022 21:49
README.md Outdated Show resolved Hide resolved
Copy link
Member

@soldni soldni left a comment

Choose a reason for hiding this comment

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

I don't know if I found them all, but suggested a few places where you can use pathlib's / syntax to join paths. From

import os
from pathlib import Path

path = Path('path/to/dir')
os.path.join(path, 'subpath/to/file.txt')

to

from pathlib import Path

path = Path('path/to/dir')
path / 'subpath/to/file.txt'

saves you an import and it's easier to read!


I've also left a comment about unifying dev and test dependency groups.

.github/workflows/mmda-ci.yml Outdated Show resolved Hide resolved
tests/test_parsers/test_pdf_plumber_parser.py Outdated Show resolved Hide resolved
tests/test_parsers/test_pdf_plumber_parser.py Outdated Show resolved Hide resolved
tests/test_parsers/test_pdf_plumber_parser.py Outdated Show resolved Hide resolved
tests/test_parsers/test_pdf_plumber_parser.py Outdated Show resolved Hide resolved
tests/test_predictors/test_figure_table_predictors.py Outdated Show resolved Hide resolved
tests/test_predictors/test_vila_predictors.py Outdated Show resolved Hide resolved
tests/test_predictors/test_vila_predictors.py Outdated Show resolved Hide resolved
tests/test_predictors/test_vila_predictors.py Outdated Show resolved Hide resolved
tests/test_predictors/test_vila_predictors.py Outdated Show resolved Hide resolved
egork520 and others added 7 commits October 20, 2022 19:05
Co-authored-by: Luca Soldaini <lucas@allenai.org>
Co-authored-by: Luca Soldaini <lucas@allenai.org>
Co-authored-by: Luca Soldaini <lucas@allenai.org>
Co-authored-by: Luca Soldaini <lucas@allenai.org>
Co-authored-by: Luca Soldaini <lucas@allenai.org>
Co-authored-by: Luca Soldaini <lucas@allenai.org>
Co-authored-by: Luca Soldaini <lucas@allenai.org>
@soldni soldni self-requested a review October 21, 2022 21:50
Copy link
Member

@soldni soldni left a comment

Choose a reason for hiding this comment

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

lgtm!

@egork520 egork520 merged commit ebda2bf into main Oct 22, 2022
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