Changed testing framework (Pytest) #275

vishav1771 · 2020-01-23T20:38:40Z

Description

Changed testing framework from Unittest to Pytest

Motivation and Context

Fixes #212

Types of Changes

Documentation update
Bug fix
New feature
Breaking change¹

¹The change breaks (or has the potential to break) existing functionality.

Checklist:

I am familiar with the Contributing Guidelines.
I agree with the terms of the Contributor Agreement.
My name is on the list of contributors.
My code follows the code style guideline of this project.
The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:

I have updated the API documentation as part of the package doc-strings.
I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
I have updated the changelog and added all related issue and pull request numbers for future reference (if applicable). See example below.

Example for a changelog entry: Fix issue with launching rockets to the moon (#101, #212).

bdice · 2020-01-24T01:30:13Z

@vishav1771 Nice work! I'm not able to review this PR right now (nor am I assigned) but I wanted to say thank you, since this is a pretty large task. 😄

vyasr · 2020-01-24T04:12:00Z

I agree, this is fantastic! I'll have a look at this as soon as possible.

csadorf

@vishav1771 Thank you for spearheading this effort! That's awesome!

I've started reviewing, but it will obviously take a bit of time to actually complete this due to the sheer volume of proposed changes. I can already promise that I will not be able to do this before the beginning of next week.

However, if you could in the meantime respond to my initial comments and change requests and also look into reasons behind the falling CI pipeline that would help accelerate the process and be very much appreciated.

tests/test_buffered_mode.py

csadorf · 2020-01-24T13:32:18Z

tests/test_buffered_mode.py


-    def test_basic_and_nested(self):
+    def test_basic_and_nested(self,setUp):


Can you help me understand the purpose of the setUp argument?

setUp is the function we want to run before running test

But it is not actually executed, is it?

tests/test_h5store.py

csadorf · 2020-01-24T15:41:33Z

Also, I forgot to mention: You've marked the PR as work-in-progress ([WIP]), GitHub has recently added a feature to mark PRs as work-in-progress directly when you open them. Would be great if you could use that feature next time, because it prevents the bot from automatically assigning reviewers and you have better control in signalling them when the PR is actually ready for review.

vishav1771 · 2020-01-25T10:32:48Z

Sure, I will keep in mind next time :)

codecov · 2020-01-25T15:20:43Z

Codecov Report

Merging #275 into master will increase coverage by 0.02%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master     #275      +/-   ##
==========================================
+ Coverage   64.91%   64.93%   +0.02%     
==========================================
  Files          40       40              
  Lines        5626     5635       +9     
==========================================
+ Hits         3652     3659       +7     
- Misses       1974     1976       +2

Impacted Files	Coverage Δ
signac/core/synceddict.py	`95.92% <100%> (+0.9%)`	⬆️
signac/contrib/import_export.py	`84.89% <100%> (+0.11%)`	⬆️
signac/contrib/project.py	`90.51% <50%> (-0.36%)`	⬇️
signac/core/jsondict.py	`95.32% <0%> (-0.59%)`	⬇️

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 745d307...8115f86. Read the comment docs.

tests/test_diff.py

tests/test_h5store.py

tests/test_job.py

tests/test_buffered_mode.py

tests/test_shell.py

tests/test_sync.py

csadorf · 2020-01-27T09:54:55Z

Great, this looks like it's almost done! There are a few minor things that should be fixed, but overall pretty good. One thing that I would like to see applied across all files is that the name pattern for BaseXTest is changed to TestXBase instead of TestBaseX.

Also, if you could make sure that the changes are meeting our formatting guidelines, which are listed in the contribution guidelines. You can use tools like flake8 and autopep8 to run an automatic check.

csadorf

You accidentally introduced a typo with this commit, otherwise good.

tests/test_project.py

csadorf · 2020-01-27T13:21:28Z

@vishav1771 Would you mind assigning yourself to the related issue: #212 ?

vishav1771 · 2020-01-27T13:59:15Z

@csadorf

@vishav1771 Would you mind assigning yourself to the related issue: #212 ?

Sorry for the inconvenience, but I am not able to assign myself this issue. I tried following the official guide, but still, no results. Can you please guide me!

bdice · 2020-01-27T14:08:43Z

@csadorf

@vishav1771 Would you mind assigning yourself to the related issue: #212 ?

Sorry for the inconvenience, but I am not able to assign myself this issue. I tried following the official guide, but still, no results. Can you please guide me!

@csadorf @vishav1771 Vishav does not have permissions to assign himself, and we cannot assign him unless he comments on the issue. Some repos have an “assigner bot” that allows users to claim issues. The bot watches for a comment like “assign me” or “take this issue” and it has write access to add them as an assignee since they have commented. More info: https://help.github.com/en/github/managing-your-work-on-github/assigning-issues-and-pull-requests-to-other-github-users

You can assign up to 10 people to each issue or pull request, including yourself, anyone who has commented on the issue or pull request, anyone with write permissions to the repository, and organization members with read permissions to the repository. For more information, see "Access permissions on GitHub."

tests/test_project.py

csadorf

Ok, great! Looks good! One last thing from my side: Can you update the instructions for test execution in the README?

csadorf

LGTM! 👍

bdice

I am requesting some changes, and I will apply those changes directly so this PR can move forward (otherwise it will block progress on some other PRs that affect tests).

README.md

bdice · 2020-01-29T14:16:23Z

changelog.txt

@@ -19,6 +19,7 @@ Changed
 +++++++

 - Workspace directory is created when ``Project`` is initialized (#267, #271).
+ - Changed testing framework from ``unittest`` to ``pytest`` (#212, #275)


Suggested change

- Changed testing framework from ``unittest`` to ``pytest`` (#212, #275)

- Changed testing framework from ``unittest`` to ``pytest`` (#212, #275).

tests/test_buffered_mode.py

bdice · 2020-01-29T14:26:56Z

tests/test_collection.py


    def test_contains(self):
-        self.assertFalse('0' in self.c)
+        assert not ('0' in self.c)


Suggested change

assert not ('0' in self.c)

assert '0' not in self.c

bdice · 2020-01-29T14:33:13Z

tests/test_collection.py

        del self.c[_id]
-        self.assertFalse(_id in self.c)
+        assert not (_id in self.c)


You might do a search for assert not to make sure that this gets changed across the whole PR. In general x not in y is better than not (x in y).

Suggested change

assert not (_id in self.c)

assert _id not in self.c

bdice · 2020-01-29T15:52:57Z

tests/test_job.py


    def test_interface_deepcopy(self):
        job = self.open_job(dict(a=0)).init()
        copy.deepcopy(job.sp).a = 1
-        self.assertFalse(job in self.project)
+        assert not (job in self.project)


Suggested change

assert not (job in self.project)

assert job not in self.project

bdice · 2020-01-29T16:07:30Z

tests/test_sync.py

@@ -2,7 +2,7 @@
 # All rights reserved.
 # This software is licensed under the BSD 3-Clause License.
 import os
-import unittest
+


Suggested change

bdice · 2020-01-29T16:14:25Z

tests/test_synced_attrdict.py

@@ -1,14 +1,15 @@
 # Copyright (c) 2017 The Regents of the University of Michigan
 # All rights reserved.
 # This software is licensed under the BSD 3-Clause License.
-import unittest
+


Suggested change

bdice · 2020-01-29T16:16:07Z

tests/test_synced_attrdict.py

@@ -516,10 +518,6 @@ def test_nested_types_dict_conversion(self):
        sad.items()
        assert type(sad['a']) is SAD
        sad.values()
-        assert type(sad['a']) is SAD
+        assert type(sad['a']), SAD


Suggested change

assert type(sad['a']), SAD

assert type(sad['a']) is SAD

bdice · 2020-01-29T16:16:26Z

tests/test_synced_attrdict.py

-
-if __name__ == '__main__':
-    unittest.main()
+        assert type(sad['a']), SAD


Suggested change

assert type(sad['a']), SAD

assert type(sad['a']) is SAD

bdice · 2020-01-29T16:22:10Z

@vishav1771 I don't have permissions to push to your fork. Can you enable "Allow edits from maintainers" on this PR?
https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork

vyasr

A couple of small changes, then this is good to go! Thanks for the huge effort on this PR.

vyasr · 2020-01-29T15:51:41Z

tests/test_buffered_mode.py

                    sleep(1.0)
                    with open(job.doc._filename, 'wb') as file:
                        file.write(json.dumps({'a': not x}).encode())
-            self.assertIn(job.doc._filename, cm.exception.files)
+            print(cm.value)
+            print(job.doc._filename)


Please remove these prints.

vyasr · 2020-01-29T15:59:25Z

tests/test_indexing.py

        collection.reset_mock()
        for fn in ('a_0.txt', 'a_1.txt'):
            os.remove(os.path.join(self._tmp_dir.name, fn))
            N = len(index)
            index = list(signac.index(root=self._tmp_dir.name, tags={'test1'}))
-            self.assertEqual(len(index), N - 1)
+            assert len(index) == N - 1


Suggested change

assert len(index) == N - 1

assert len(index) == (N - 1)

vyasr · 2020-01-29T16:01:25Z

tests/test_jsondict.py

+        assert bool(jsd)
+        assert len(jsd) == 1
+        assert key in jsd
+        assert key in jsd


Can we delete this line? This test is identical to the one above.

No, this is deliberately testing twice.

I need to correct myself, doesn't look like this duplication here is crucial.

vishav1771 · 2020-01-29T16:59:30Z

@vishav1771 I don't have permissions to push to your fork. Can you enable "Allow edits from maintainers" on this PR?

Enabled :)

bdice

Thanks again @vishav1771, I think this is complete.

vishav1771 added 2 commits January 24, 2020 00:20

unittest to pytest

916056a

Added CleanUp

85a7756

vishav1771 requested review from a team as code owners January 23, 2020 20:38

vishav1771 requested review from csadorf and removed request for a team January 23, 2020 20:38

vishav1771 changed the title ~~Changed testing framework (Pytest)~~ [WIP]Changed testing framework (Pytest) Jan 23, 2020

vishav1771 requested a review from vyasr January 23, 2020 20:40

bdice assigned vishav1771 Jan 24, 2020

bdice added the enhancement New feature or request label Jan 24, 2020

csadorf requested changes Jan 24, 2020

View reviewed changes

vishav1771 added 6 commits January 25, 2020 16:47

Added autouse

65fcfc3

Minor changes

00cf456

Error Fix

e4b0b9c

Error Fix

afacf13

pyetst error fix

b6cc5e3

Changed Fixtures

745d307

vishav1771 added 2 commits January 25, 2020 22:25

Removing Conflicts

e32d9fd

Error

e91cfb2

vishav1771 requested a review from csadorf January 25, 2020 17:09

vishav1771 changed the title ~~[WIP]Changed testing framework (Pytest)~~ Changed testing framework (Pytest) Jan 25, 2020

csadorf reviewed Jan 27, 2020

View reviewed changes

csadorf requested changes Jan 27, 2020

View reviewed changes

tests/test_shell.py Show resolved Hide resolved

tests/test_shell.py Outdated Show resolved Hide resolved

tests/test_sync.py Outdated Show resolved Hide resolved

typo

a31d567

csadorf requested changes Jan 27, 2020

View reviewed changes

tests/test_project.py Outdated Show resolved Hide resolved

typo

950fadc

vishav1771 requested a review from csadorf January 27, 2020 14:22

csadorf requested changes Jan 27, 2020

View reviewed changes

tests/test_project.py Outdated Show resolved Hide resolved

typo :)

9b63363

vishav1771 requested a review from csadorf January 27, 2020 14:56

csadorf requested changes Jan 27, 2020

View reviewed changes

Modified README

9938789

vishav1771 requested a review from csadorf January 27, 2020 16:58

csadorf approved these changes Jan 27, 2020

View reviewed changes

bdice added this to the v1.4.0 milestone Jan 27, 2020

bdice mentioned this pull request Jan 27, 2020

Fix project data interface and add unittests for job and project data interfaces #278

Merged

12 tasks

vishav1771 requested review from bdice and removed request for vyasr January 29, 2020 14:06

bdice requested changes Jan 29, 2020

View reviewed changes

vyasr requested changes Jan 29, 2020

View reviewed changes

vishav1771 added 2 commits January 29, 2020 22:36

Changes

06c8a77

Minor Change

8115f86

vishav1771 requested review from vyasr and bdice January 29, 2020 17:36

bdice approved these changes Jan 29, 2020

View reviewed changes

vyasr approved these changes Jan 29, 2020

View reviewed changes

vyasr merged commit c9810b9 into glotzerlab:master Jan 29, 2020

bdice mentioned this pull request Jan 29, 2020

Adopt the pytest library for testing glotzerlab/signac-flow#227

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed testing framework (Pytest) #275

Changed testing framework (Pytest) #275

vishav1771 commented Jan 23, 2020 •

edited by csadorf

Loading

bdice commented Jan 24, 2020

vyasr commented Jan 24, 2020

csadorf left a comment

csadorf Jan 24, 2020

vishav1771 Jan 25, 2020

csadorf Jan 27, 2020 •

edited

Loading

csadorf commented Jan 24, 2020

vishav1771 commented Jan 25, 2020

codecov bot commented Jan 25, 2020 •

edited

Loading

csadorf commented Jan 27, 2020

csadorf left a comment

csadorf commented Jan 27, 2020

vishav1771 commented Jan 27, 2020

bdice commented Jan 27, 2020

csadorf left a comment

csadorf left a comment

bdice left a comment

bdice Jan 29, 2020

bdice Jan 29, 2020

bdice Jan 29, 2020

bdice Jan 29, 2020

bdice Jan 29, 2020

bdice Jan 29, 2020

bdice Jan 29, 2020

bdice Jan 29, 2020

bdice commented Jan 29, 2020

vyasr left a comment

vyasr Jan 29, 2020

vyasr Jan 29, 2020

vyasr Jan 29, 2020

csadorf Jan 29, 2020

csadorf Jan 29, 2020

vishav1771 commented Jan 29, 2020

bdice left a comment


		def test_basic_and_nested(self):
		def test_basic_and_nested(self,setUp):

	- Changed testing framework from ``unittest`` to ``pytest`` (#212, #275)
	- Changed testing framework from ``unittest`` to ``pytest`` (#212, #275).

	assert not (job in self.project)
	assert job not in self.project

Changed testing framework (Pytest) #275

Changed testing framework (Pytest) #275

Conversation

vishav1771 commented Jan 23, 2020 • edited by csadorf Loading

Description

Motivation and Context

Types of Changes

Checklist:

bdice commented Jan 24, 2020

vyasr commented Jan 24, 2020

csadorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csadorf Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

csadorf commented Jan 24, 2020

vishav1771 commented Jan 25, 2020

codecov bot commented Jan 25, 2020 • edited Loading

Codecov Report

csadorf commented Jan 27, 2020

csadorf left a comment

Choose a reason for hiding this comment

csadorf commented Jan 27, 2020

vishav1771 commented Jan 27, 2020

bdice commented Jan 27, 2020

csadorf left a comment

Choose a reason for hiding this comment

csadorf left a comment

Choose a reason for hiding this comment

bdice 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

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

bdice commented Jan 29, 2020

vyasr 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

Choose a reason for hiding this comment

vishav1771 commented Jan 29, 2020

bdice left a comment

Choose a reason for hiding this comment

vishav1771 commented Jan 23, 2020 •

edited by csadorf

Loading

csadorf Jan 27, 2020 •

edited

Loading

codecov bot commented Jan 25, 2020 •

edited

Loading