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.

Sign up for GitHub

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

Jump to bottom

Adds unit tests #41

Merged
merged 29 commits into from
Aug 25, 2021
Merged

Adds unit tests #41

merged 29 commits into from
Aug 25, 2021

Conversation

NishanthJKumar
Copy link
Contributor

Includes 1 E2E test, as well as a few files with basic tests for some of our other functionalities. These other tests are incomplete and intended to show a proposal for how we might structure all our test files. Would love to get feedback on (1) test structuring and (2) which files/functions we should create tests for, and which ones will just be unnecessary

Resolves #11

Nishanth and others added 5 commits July 30, 2021 09:34
@NishanthJKumar NishanthJKumar self-assigned this Aug 2, 2021
@StannisZhou
Copy link
Contributor

StannisZhou commented Aug 4, 2021 •
edited
Loading

@wlehrach Any comments/suggestions for unit test strategy? What should we cover? How should we cover them?
@lazarox I remember you had/suggested some tests before. What are those?

Nishanth and others added 20 commits August 13, 2021 14:43
Merge branch 'master' of github.com:vicariousinc/PGMax into vicarious…
0835ef8 
…inc-master
Merge branch 'vicariousinc-master' into unit_tests
407ccee 
updates unit tests after deletion of old interface
updates sanity check and E2E test, but test currently fails
2c7ab3c
@NishanthJKumar
adds coverage dependency to poetry
99501b4
@NishanthJKumar
Merge branch 'unit_tests' into master
e0460e7
@NishanthJKumar
Merge pull request #7 from NishanthJKumar/master
acaf801 
New Interface for Factor Graph Model Building (#53)
@NishanthJKumar
small update to test_pgmax
a6a32ff
@NishanthJKumar
adds codecov to CI
f1811aa
@NishanthJKumar
test codecov upload with github secret
bf78603
@NishanthJKumar
updates ci and poetry deps
944297d 
* ci now uses pytest-cov to generate reports
* removes coverage requirement from poetry dev reqs
* small update to codecov yml
@NishanthJKumar
fixes command error in ci
e2afc5f
@NishanthJKumar
fixes codecov yaml to be parseable
46188df
@NishanthJKumar
Merge branch 'master' of github.com:vicariousinc/PGMax into vicarious…
c5aa087 
…inc-master
@NishanthJKumar
Merge branch 'vicariousinc-master' into unit_tests
889479c 
Merging after heretic model incorporation
@NishanthJKumar
updates ci to only run build on PR's again!
cbac54a
@NishanthJKumar
testing codecov from personal fork
db8f120
@NishanthJKumar
updates tests
8472fd4 
* gets 100% coverage for nodes.py
* updates poetry to depend on strictly python 3.7 (others cause problems with pre-commit, etc.)
* minor update to gitignore
@NishanthJKumar
adds simple e2e test inspired by the heretic model
43ccf99
@NishanthJKumar
includes tests reaching 95% coverage of pgmax
2ab9fbe
@NishanthJKumar
adds more tests to get to 97% overall coverage
d75050e
@NishanthJKumar
updates tests to 100% coverage
ed293af 
* updates ci to run correct testing command
* suppresses coverage checking for certain functions that cannot possibly be run (in groups.py)
@NishanthJKumar
Copy link
Contributor Author

Coverage is now 100% - ready for review and potential merging!

Note that I had to tell pytest-cov to ignore certain functions (with the # pragma: no cover comment) that can never be run (e.g. they're supposed to be inherited or just contain pass statements).

StannisZhou
StannisZhou reviewed Aug 24, 2021
View reviewed changes
Copy link
Contributor

@StannisZhou StannisZhou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One major comment: instead of relying on manual annotation for lines we should ignore in coverage calculation, let's replace pass with docstrings and ignore NotImplementedError lines.

Also made various minor comments.

.github/workflows/ci.yaml Show resolved Hide resolved
pgmax/fg/groups.py Outdated Show resolved Hide resolved
pgmax/fg/groups.py Outdated Show resolved Hide resolved
pgmax/fg/groups.py Show resolved Hide resolved
pgmax/fg/nodes.py Outdated Show resolved Hide resolved
pytest.ini Outdated Show resolved Hide resolved
tests/fg/test_graph.py Outdated Show resolved Hide resolved
tests/fg/test_groups.py Outdated Show resolved Hide resolved
tests/fg/test_groups.py Show resolved Hide resolved
tests/test_pgmax.py Outdated Show resolved Hide resolved
@NishanthJKumar NishanthJKumar marked this pull request as ready for review August 25, 2021 18:49
StannisZhou
StannisZhou approved these changes Aug 25, 2021
View reviewed changes
Copy link
Contributor

@StannisZhou StannisZhou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after addressing a few more minor comments

pgmax/fg/groups.py Outdated Show resolved Hide resolved
pgmax/fg/groups.py Outdated Show resolved Hide resolved
pgmax/fg/groups.py Outdated Show resolved Hide resolved
pgmax/fg/groups.py Outdated Show resolved Hide resolved
tests/fg/test_groups.py Outdated Show resolved Hide resolved
@NishanthJKumar NishanthJKumar merged commit f8555a4 into vicariousinc:master Aug 25, 2021
@NishanthJKumar NishanthJKumar deleted the unit_tests branch August 25, 2021 20:39
@NishanthJKumar NishanthJKumar mentioned this pull request Aug 25, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StannisZhou StannisZhou StannisZhou approved these changes

@wlehrach wlehrach Awaiting requested review from wlehrach

@lazarox lazarox Awaiting requested review from lazarox

Assignees

@NishanthJKumar NishanthJKumar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test sanity check example using new interface and inference modules, and put together the first unit test
2 participants
@NishanthJKumar @StannisZhou