Tests for feedstock_io #357

jakirkham · 2016-11-02T00:09:32Z

This is a follow-up on PR ( #336 ).

Adds a bunch of tests for the feedstock_io module. Tests exist for both the presence and absence of a git repo. The combination of these should help improve test coverage of conda-smithy particularly on these low level aspects.

cc @nicoddemus

nicoddemus · 2016-11-02T01:53:54Z

Very nice! I will find some time to review this tomorrow.

Let me ask, would you guys consider using pytest for the tests? If it is a possibility, I could rewrite this module using pytest so you can compare and see if it is worth it. Also, pytest can run unittest's tests out of the box so it is easy to switch over.

nicoddemus · 2016-11-03T14:55:00Z

conda_smithy/tests/test_feedstock_io.py

+        self.assertEqual(file_mode & set_mode, set_mode)
+
+
+    def test_set_mode_file_2(self):


test_set_mode_file_1 and test_set_mode_file_2 are very similar, I think we can remove one of them.

Yeah, so I had added duplicates of these as I started running into issues with absolute paths vs relative paths locally. Have made some fixes to conda-smithy to handle these, but felt they should be tested somehow.

Oh I see, I guess we can leave them in this case.

Those would be a prime candidate for parametrization if we were using pytest, btw. 😉

Should add this is what is going on with all the other *_1 and *_2. Definitely agree that it is kind of obnoxious to have so many like this.

nicoddemus · 2016-11-03T14:55:51Z

conda_smithy/tests/test_feedstock_io.py

+        with fio.write_file(filename) as fh:
+            fh.write(write_text)
+
+        read_text = ""


Small nit: this line is not needed

True throughout. It's my C/C++ programmer side getting in the way. To use to thinking about scope in this way. Can drop them if you wish.

Up to you. 😁

nicoddemus · 2016-11-03T14:55:56Z

conda_smithy/tests/test_feedstock_io.py

+        with fio.write_file(filename) as fh:
+            fh.write(write_text)
+
+        read_text = ""


Small nit: this line is not needed

nicoddemus · 2016-11-03T14:56:00Z

conda_smithy/tests/test_feedstock_io.py

+        with fio.write_file(filename) as fh:
+            fh.write(write_text)
+
+        read_text = ""


Small nit: this line is not needed

nicoddemus · 2016-11-03T14:56:04Z

conda_smithy/tests/test_feedstock_io.py

+
+        fio.touch_file(filename)
+
+        read_text = ""


Small nit: this line is not needed

nicoddemus · 2016-11-03T15:35:29Z

conda_smithy/tests/test_feedstock_io.py

+        with fio.write_file(filename) as fh:
+            fh.write(write_text)
+
+        read_text = ""


Small nit: this line is not needed

nicoddemus · 2016-11-03T15:35:35Z

conda_smithy/tests/test_feedstock_io.py

+
+        fio.touch_file(filename)
+
+        read_text = ""


Small nit: this line is not needed

nicoddemus · 2016-11-03T15:35:42Z

conda_smithy/tests/test_feedstock_io.py

+
+        fio.touch_file(filename)
+
+        read_text = ""


Small nit: this line is not needed

nicoddemus · 2016-11-03T15:36:19Z

conda_smithy/tests/test_feedstock_io.py

+        filename = "dir1/dir2/test.txt"
+        dirname = os.path.dirname(filename)
+
+        if dirname and not os.path.exists(dirname):


Small nit: this line is not needed, the directory will never exist because setUp creates a fresh directory

This was probably some copypasta. Good catch.

Fixed. 😄

nicoddemus · 2016-11-03T15:36:27Z

conda_smithy/tests/test_feedstock_io.py

+        self.assertTrue(os.path.exists(filename2))
+        self.assertTrue(list(self.repo.index.iter_blobs(BlobFilter(filename2))))
+
+        read_text = ""


Small nit: this line is not needed

jakirkham · 2016-11-03T16:30:52Z

Let me ask, would you guys consider using pytest for the tests? If it is a possibility, I could rewrite this module using pytest so you can compare and see if it is worth it. Also, pytest can run unittest's tests out of the box so it is easy to switch over.

Sorry I hadn't answered this yet. So my primary concern is that the test coverage is miserable in conda-smithy. Any way we can improve that gets a 👍 from me.

I think we were running the test suite with nose before, but AFAICT everything uses unittest. Also did a little cleanup ( #355 ) so that we don't really need nose at all.

Have to confess I really don't have much experience with pytest having used nose or unittest in the past. Though am definitely interested in learning. The vast number of plugins for pytest gives me hope that it is really flexible under the hood. Have some other non-conda-forge activities that would really benefit from such a flexible testing framework.

nicoddemus · 2016-11-03T16:35:20Z

Have to confess I really don't have much experience with pytest having used nose or unittest in the past.

Sure. IMHO using pytest just as a test runner is useful, because you can use plain assert statements instead of having to use self.assert* functions.

The vast number of plugins for pytest gives me hope that it is really flexible under the hood. Have some other non-conda-forge activities that would really benefit from such a flexible testing framework.

IMHO it is very flexible, but of course as a maintainer that would be my opinion. 😁

If it is alright with you then, I will work on a PR that makes conda-smithy use pytest as test runner, and convert some (or all) of the tests to pytest's style. Sounds good?

jakirkham · 2016-11-03T17:30:28Z

Go for it. 👍 Feel free to pull these tests in too if you wish.

It'd be nice if @pelson could weigh in as I don't know his preferences, but I think he is otherwise engaged ATM.

jakirkham · 2016-11-05T21:27:13Z

Have rebased this now that coverage is being checked by Coveralls.

jakirkham · 2016-11-05T22:54:12Z

Thanks for your feedback @nicoddemus .

Have consolidated the various parameterizations of these tests into single unit test functions that loop over those permutations. Hopefully this keeps the maintenance burden low.

Will go ahead and put this in for now so we can ensure these functions remain correct. Though still would be interested to see your pytest version of this and/or other tests in conda-smithy.

nicoddemus · 2016-11-06T16:09:09Z

Though still would be interested to see your pytest version of this and/or other tests in conda-smithy.

Sure, will do probably this week! 😁

jakirkham changed the title ~~Test for feedstock_io~~ Tests for feedstock_io Nov 2, 2016

jakirkham mentioned this pull request Nov 2, 2016

Handle empty directories #353

Merged

jakirkham force-pushed the test_feedstock_io branch 8 times, most recently from 3c2571a to 6b9bdf0 Compare November 2, 2016 05:17

nicoddemus reviewed Nov 3, 2016

View reviewed changes

jakirkham force-pushed the test_feedstock_io branch 3 times, most recently from 9cea999 to 2f55bf2 Compare November 5, 2016 21:23

Add a stub for testing feedstock_io.

10b53c3

jakirkham force-pushed the test_feedstock_io branch from ec89542 to 9e0f7e3 Compare November 5, 2016 22:38

jakirkham added 9 commits November 5, 2016 18:40

Setup test framework for feedstock_io when a repo is not present.

5ed7095

Add some tests for get_repo.

89c1530

Add some tests for get_file_blob.

fd443f8

Add some tests for get_mode_file.

6d0173a

Add some tests for set_mode_file.

46ed288

Add some tests for write_file.

55731e1

Add some tests for touch_file.

cbd264f

Add some tests for remove_file.

94b0761

Add some tests for copy_file.

21221c2

jakirkham force-pushed the test_feedstock_io branch from 9e0f7e3 to 21221c2 Compare November 5, 2016 22:49

jakirkham merged commit d60880c into conda-forge:master Nov 5, 2016

jakirkham deleted the test_feedstock_io branch November 5, 2016 22:54

nicoddemus mentioned this pull request Nov 15, 2016

Use Pytest #385

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests for feedstock_io #357

Tests for feedstock_io #357

jakirkham commented Nov 2, 2016

nicoddemus commented Nov 2, 2016

nicoddemus Nov 3, 2016

jakirkham Nov 3, 2016

nicoddemus Nov 3, 2016

jakirkham Nov 3, 2016

nicoddemus Nov 3, 2016

jakirkham Nov 3, 2016

nicoddemus Nov 3, 2016

nicoddemus Nov 3, 2016

nicoddemus Nov 3, 2016

nicoddemus Nov 3, 2016

nicoddemus Nov 3, 2016

nicoddemus Nov 3, 2016

nicoddemus Nov 3, 2016

nicoddemus Nov 3, 2016

jakirkham Nov 3, 2016

jakirkham Nov 5, 2016

nicoddemus Nov 3, 2016

jakirkham commented Nov 3, 2016

nicoddemus commented Nov 3, 2016

jakirkham commented Nov 3, 2016

jakirkham commented Nov 5, 2016

jakirkham commented Nov 5, 2016

nicoddemus commented Nov 6, 2016

		self.assertEqual(file_mode & set_mode, set_mode)


		def test_set_mode_file_2(self):

Tests for feedstock_io #357

Tests for feedstock_io #357

Conversation

jakirkham commented Nov 2, 2016

nicoddemus commented Nov 2, 2016

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Nov 3, 2016

nicoddemus commented Nov 3, 2016

jakirkham commented Nov 3, 2016

jakirkham commented Nov 5, 2016

jakirkham commented Nov 5, 2016

nicoddemus commented Nov 6, 2016