Add tests for copying. Setup project for pytest. #88

pyasi · 2018-10-18T03:52:24Z

Added tests for critical paths in the copying functionality
Made a small refactor to the INVALID dirs to be a constant. Thus they can be referenced anywhere and changed in one place
Used pytest as requested in Testing Suite #69

This is just a start to the testing journey possibilities! But wanted to get my feet wet in the repository.

alichtman

Great start on the tests! Check out the comments I left and let me know if you have any questions.

alichtman · 2018-10-18T05:52:16Z

constants.py

@@ -7,3 +7,5 @@ class Constants:
 	URL='https://github.com/alichtman/shallow-backup'
 	AUTHOR_EMAIL='aaronlichtman@gmail.com'
 	CONFIG_PATH='.shallow-backup'
+	INVALIDS = [".Trash", ".npm", ".cache", ".rvm"]


Let's make the name a bit more descriptive, but this is a good refactor.

Suggested change

INVALIDS = [".Trash", ".npm", ".cache", ".rvm"]

INVALID_DIRS = [".Trash", ".npm", ".cache", ".rvm"]

alichtman · 2018-10-18T05:52:34Z

shallow_backup.py

@@ -121,7 +121,7 @@ def copy_dir(source_dir, backup_path):
 	"""
 	Copy dotfolder from $HOME.
 	"""
-	invalid = {".Trash", ".npm", ".cache", ".rvm"}
+	invalid = set(Constants.INVALIDS)


Suggested change

invalid = set(Constants.INVALIDS)

invalid = set(Constants.INVALID_DIRS)

shallow_backup.py

alichtman · 2018-10-18T05:59:09Z

tests/test_copies.py

+from shallow_backup import _copy_file, copy_dir
+from constants import Constants
+
+TEST_DIR = 'test-directory'


Let's rename these all to be shallow-backup-... to decrease the chance of conflicting file/directory names and accidentally destroying someone's data.

And shorten directory to just dir

alichtman · 2018-10-18T06:03:18Z

tests/test_copies.py

+        Test that copying a directory works as expected
+        """
+        dir_to_copy = '/test/'
+        os.mkdir(TEST_DIR + dir_to_copy)


It's better practice to use os.path.join() for creating paths since that's cross-platform compatible. Let's do that here.

tests/test_copies.py

alichtman · 2018-10-18T06:09:03Z

tests/test_copies.py

+        for directory in DIRS:
+            try:
+                os.mkdir(directory)
+            except FileExistsError:


If the directories exist already, instead of removing them immediately (since the teardown methods should remove them, so if they exist, it's possible the directories weren't created by us), we should print an error message and ask the user whether they want to remove the files or exit, using the inquirer library in the same style as the CLI.

Although I'm open to talk about this one. With the file name change to be much less generic, the prompt may be overkill and could interfere with scripting. What do you think?

Yeah I did this so that if it was ever run in a CI environment we shouldn't expect to have any issues. A prompt in a CI environment would hang the build.

We can try to be smarter about this and append the current datetime to definitely avoid conflicts and then remove all dirs with a suffix shallow-copy-dir-*

TBH I think that might be a bit of over-engineering for a simple unit test solution. Going to keep as is for now but I'm happy to throw it is (simple change) if you deem necessary

We can try to be smarter about this and append the current datetime to definitely avoid conflicts and then remove all dirs with a suffix shallow-copy-dir-*

~~This is a great solution -- let's do it.~~

After further consideration, let's do shallow-backup-test-dir with no date after it because it's unlikely that someone has a pre-existing dir named that which isn't used by shallow-backup.

alichtman · 2018-10-18T06:12:10Z

Pipfile

@@ -8,6 +8,7 @@ click = "*"
 inquirer = "*"
 gitpython = "*"
 colorama = "*"
+pytest = "*"


Shouldn't there be a Pipfile.lock update, as well? Can you commit that?

I took care of the Pipfile changes in my previous PR, so this is on master already. You should remove the Pipfile and Pipfile.lock files from your commit (if they differ from the ones I committed. If they're the same, git will just take care of it.)

…low-backup into add_tests_for_copying

pyasi · 2018-10-18T13:28:22Z

@alichtman I addressed all comments besides the ones I explained. Let me know if you still have concerns on those choices.

alichtman · 2018-10-18T21:12:44Z

Two minor changes and we can get this merged in, @pyasi

alichtman · 2018-10-18T21:34:54Z

tests/test_copies.py

+from shallow_backup import _copy_file, _copy_dir
+from constants import Constants
+
+TEST_DIR = 'shallow-backup-test-dir'


Let's name one of these TEST_DIR_FOR_BACKUP and one of them TEST_DIR_TO_BACKUP so there's no confusion about which is which.

For future test improvements.

alichtman · 2018-10-18T22:24:12Z

In reading through these tests again, there are some definite improvements that could be made. I've added TODOs for these in the code, but I wanted to get the structure merged in so other people to work on it, as well. I've opened a bunch of other testing issues, if you're interested in taking a look at those. I think it's really important to have a comprehensive testing suite for this, and I'll work on building it out over time, but the help right now is definitely appreciated.

Again, thanks for the contribution. I'd be excited to work on some more features with you in the future.

pyasi · 2018-10-18T22:47:21Z

One of your test changes broke a test. Let's get in the habit of running the suite before commiting/merging. I'll take a look at some of the other testable pieces.

alichtman · 2018-10-18T22:50:16Z

Let's get in the habit of running the suite before commiting/merging.

You're totally correct. I did a quick refactor in the GitHub editor without actually pulling it to test. That was sloppy...

Is the error on line 42? test_dir = 'test/' was changed to test_dir = 'test'.

pyasi and others added 7 commits October 17, 2018 23:25

Add tests for the copying functionality

9eb72e9

Refactor invalids to be a constant. DRY.

b0f019a

Update contirbutors and add a contributing step to run tests

bed0f62

Use pytest instead as requested in issue

87d06cf

Update instructions for running tests

ffc5e61

Update CONTRIBUTING.md

85f70fe

Update CONTRIBUTING.md

19ebe3a

alichtman requested changes Oct 18, 2018

View reviewed changes

pyasi added 2 commits October 18, 2018 09:27

Make adjustments to address CR comments

fe9e321

Merge branch 'add_tests_for_copying' of https://github.com/pyasi/shal…

23b87a1

…low-backup into add_tests_for_copying

alichtman closed this Oct 18, 2018

alichtman reopened this Oct 18, 2018

Fix usage of os.path.join

4b733d8

alichtman reviewed Oct 18, 2018

View reviewed changes

pyasi and others added 2 commits October 18, 2018 18:09

Clean up variable names

e8ed2ea

Refactor naming and add todos...

ff80546

For future test improvements.

alichtman merged commit 8516612 into alichtman:master Oct 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for copying. Setup project for pytest. #88

Add tests for copying. Setup project for pytest. #88

pyasi commented Oct 18, 2018

alichtman left a comment

alichtman Oct 18, 2018

alichtman Oct 18, 2018

alichtman Oct 18, 2018

alichtman Oct 18, 2018

alichtman Oct 18, 2018 •

edited

Loading

alichtman Oct 18, 2018

alichtman Oct 18, 2018

pyasi Oct 18, 2018

pyasi Oct 18, 2018

alichtman Oct 18, 2018 •

edited

Loading

alichtman Oct 18, 2018 •

edited

Loading

alichtman Oct 18, 2018

alichtman Oct 18, 2018

pyasi commented Oct 18, 2018

alichtman commented Oct 18, 2018 •

edited

Loading

alichtman Oct 18, 2018

alichtman commented Oct 18, 2018 •

edited

Loading

pyasi commented Oct 18, 2018

alichtman commented Oct 18, 2018 •

edited

Loading

	INVALIDS = [".Trash", ".npm", ".cache", ".rvm"]
	INVALID_DIRS = [".Trash", ".npm", ".cache", ".rvm"]

	invalid = set(Constants.INVALIDS)
	invalid = set(Constants.INVALID_DIRS)

Add tests for copying. Setup project for pytest. #88

Add tests for copying. Setup project for pytest. #88

Conversation

pyasi commented Oct 18, 2018

alichtman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alichtman Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alichtman Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

alichtman Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyasi commented Oct 18, 2018

alichtman commented Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

alichtman commented Oct 18, 2018 • edited Loading

pyasi commented Oct 18, 2018

alichtman commented Oct 18, 2018 • edited Loading

alichtman Oct 18, 2018 •

edited

Loading

alichtman Oct 18, 2018 •

edited

Loading

alichtman Oct 18, 2018 •

edited

Loading

alichtman commented Oct 18, 2018 •

edited

Loading

alichtman commented Oct 18, 2018 •

edited

Loading

alichtman commented Oct 18, 2018 •

edited

Loading