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

[Issue 179] Adding a test for the osprojects model #184

Merged
merged 4 commits into from
Oct 2, 2020

Conversation

lpatmo
Copy link
Member

@lpatmo lpatmo commented Sep 12, 2020

What type of PR is this? (check all applicable)

  • πŸ› Bug Fix
  • ✨ Feature (e.g. API change, enhancement, addition, etc.)
  • ♻️ Refactor
  • πŸ“ Documentation
  • πŸ”– Release
  • 🚩 Other (concerning: tests)

Context

Tests the osprojects model we created! Two todos:

[x] Cleared up this issue: osprojects/tests.py::test_osprojects_model /usr/local/lib/python3.7/site-packages/django/db/models/fields/__init__.py:1363: RuntimeWarning: DateTimeField OSProjects.modified received a naive datetime (2020-05-31 00:00:00) while time zone support is active. RuntimeWarning) by using datetime.now()
[ ] Add more asserts (maybe?)

Other Related Tickets & Documents (as needed)

#179

Implementation Details

[x] Created a factories.py file to generate new osprojects
[x] Tested that a newly created osproject had the relevant fields
[x] Ran docker-compose run --rm app pytest and confirmed that tests passed

New Libraries/Dependancies Introduced (Fill out as needed)

N/A

If you have added libraries or other dependancies, please list them (and links to their repos) below:

  • I've updated requirements.txt

Any new migration files added?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed

Did you add tests?

Code added or changed without test coverage or good reason for exemption won't be approved.

Yes, but could use feedback on these tests!

  • πŸ‘ yes
  • πŸ™‹ no, because I need help
  • πŸ™… no, because they aren't needed

Did you add documentation?

  • πŸ“œ readme.md
  • πŸ“œ contributing.md
  • πŸ“œ wiki entry
  • πŸ™‹ I'd like someone to help write documentation, and will file a new issue for it
  • πŸ™… no documentation needed

@codecov
Copy link

codecov bot commented Sep 12, 2020

Codecov Report

❗ No coverage uploaded for pull request base (main@649c0c1). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #184   +/-   ##
=======================================
  Coverage        ?   82.21%           
=======================================
  Files           ?       34           
  Lines           ?      568           
  Branches        ?        0           
=======================================
  Hits            ?      467           
  Misses          ?      101           
  Partials        ?        0           

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 649c0c1...ce4153b. Read the comment docs.

@@ -7,6 +7,7 @@
from tagging.managers import CustomTaggableManager
from tagging.models import CustomTag, TaggedItems


Copy link
Member Author

Choose a reason for hiding this comment

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

Note: my linter caused this extra line, but not sure why it's appearing πŸ€”

Copy link
Member

Choose a reason for hiding this comment

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

Apologies for the brain fart and not reviewing this sooner! I think your linter is enforcing some sort of PEP 8 thing where imports are separated from code by two lines? Dunno. Weird.

""" Test OSProjects model """
# create OSProjects model instance
osproject = OSProjectsFactory(title="Buddybot", guid="cc63f918-d515-11ea-956f-0242ac130002")
assert osproject.title == "Buddybot"
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: was thinking I could add more asserts here, but was wondering if this might also be fine

Copy link
Member

@BethanyG BethanyG Sep 29, 2020

Choose a reason for hiding this comment

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

Up to you. I usually go for an entire record -- or even two, then pick up to four things to assert. But even one assert proves it was inserted into the DB, which is what we are going for here....so.....

@lpatmo lpatmo changed the title [Issue 179] Adding a few model tests [Issue 179] Adding a test for the osprojects model Sep 13, 2020
Copy link
Member

@BethanyG BethanyG left a comment

Choose a reason for hiding this comment

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

I think this works. You could get more elaborate, or even update data and check to make sure it works. Although that's getting into re-testing the ORM territory. I think this is probably "good enough" for a first pass at a model test for OSProjects. We can revisit as we get more of the endpoint done.

@lpatmo lpatmo merged commit fb77652 into main Oct 2, 2020
@lpatmo
Copy link
Member Author

lpatmo commented Oct 2, 2020

Thank you for reviewing, @BethanyG!! Will merge this in... and as you said, revisit/update as we get more of the endpoint done.

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