-
Notifications
You must be signed in to change notification settings - Fork 1
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
REF: Refactor tests #39
Conversation
Codecov Report
@@ Coverage Diff @@
## release/0.0.14 #39 +/- ##
==================================================
- Coverage 86.20% 85.83% -0.37%
==================================================
Files 7 7
Lines 232 233 +1
==================================================
Hits 200 200
- Misses 32 33 +1
Continue to review full report at Codecov.
|
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'm really happy with the refactor so far, thank you. Nothing crazy but it does make things a lot tidier.
On the file deletion, I've made a note on the concept of deserialisation, which it'd be important to understand. In general, I think the most straightforward thing would be to take the test fixtures out of files and put them in """{...}"""
strings rather than converting to objects.
One thing I've realised I'm doing is that I'm copying/pasting the valid_*.json twines in the test cases as bases for building my own twines. They probably would be better off as examples in the documentation or elsewhere though, so I'm not willing to cling on to the file structure for that reason.
tests/base.py
Outdated
def setUp(self): | ||
self.path = str(os.path.join(os.path.dirname(os.path.abspath(__file__)), "data", "")) | ||
super().setUp() | ||
path = os.path.join(os.path.dirname(__file__), "data") |
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.
Well simplified path. The reason I used abspath
and str
sprang from refactoring twined out of the old Octue SDK where the CLI required absolute string paths. Assuming tests still pass, this is fine now.
I prefer to keep the setUp()
pattern in general, because that sets path
on the instance, not on the class. Any subclasses can therefore modify self.path
without potential-weirdness affecting other tests running in parallel.
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.
As far as I'm aware, __file__
is an absolute path already and os.path.join
always creates a string - let me know if this isn't the case!
Good point, I'll put it back in setUp
👍
Summary
BaseTestCase
path attributeos.path.join
in testsIn
tests.test_children
:Twine
instantiation, allowing test cases to be brought directly in to testsTwine
instantiation (I didn't want to change further test files without checking first!)