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

REF: Refactor tests #39

Merged
merged 9 commits into from
Nov 2, 2020
Merged

REF: Refactor tests #39

merged 9 commits into from
Nov 2, 2020

Conversation

cortadocodes
Copy link
Member

@cortadocodes cortadocodes commented Oct 10, 2020

Summary

  • Simplify BaseTestCase path attribute
  • Use os.path.join in tests
  • Remove simple single-use variables from tests

In tests.test_children:

  • Allow python object input in Twine instantiation, allowing test cases to be brought directly in to tests
  • Bring test cases directly in to tests, allowing the entire test to be seen in one go in one test method (rather than having to look at two files to see what is being tested), and avoiding the need for parsing from JSON to python object in the test suite
  • Shorten strings in test cases so they span fewer lines
  • I've done this only to this module as I thought there might be a reason that I've missed to keep the test cases as JSON files or to not allow python objects to be used as the source in Twine instantiation (I didn't want to change further test files without checking first!)

@codecov
Copy link

codecov bot commented Oct 10, 2020

Codecov Report

Merging #39 into release/0.0.14 will decrease coverage by 0.36%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                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     
Impacted Files Coverage Δ
twined/twine.py 78.57% <100.00%> (-0.52%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82376eb...31e99c2. Read the comment docs.

@cortadocodes cortadocodes marked this pull request as ready for review October 10, 2020 17:31
@cortadocodes cortadocodes changed the base branch from master to release/0.0.14 October 10, 2020 17:31
@cortadocodes cortadocodes changed the title Refactor tests REF: Refactor tests Oct 11, 2020
Copy link
Collaborator

@thclark thclark left a 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")
Copy link
Collaborator

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.

Copy link
Member Author

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 👍

tests/test_children.py Outdated Show resolved Hide resolved
@thclark thclark merged commit f099ec0 into release/0.0.14 Nov 2, 2020
@thclark thclark deleted the refactor-tests branch November 2, 2020 09:57
@thclark thclark restored the refactor-tests branch November 2, 2020 16:13
@thclark thclark deleted the refactor-tests branch November 2, 2020 16:24
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