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

feat: Creates tests for the example app - Redo #59

Merged
merged 10 commits into from
Jun 7, 2021

Conversation

hmanalai
Copy link
Contributor

@hmanalai hmanalai commented Jun 3, 2021

Adds three new files that tests example app's models, /me, and /ping endpoints.

@bgrant
Copy link
Collaborator

bgrant commented Jun 4, 2021

Hey @hmanalai - thanks! A couple of high-level suggestions:

  • Please add a line to the CHANGELOG describing your change
  • I'd really like these tests to run as part of the test-all Make target (referenced in Makefile in the root of this project) so that the tests run as part of Pull Request checks. We'd like to know if we break the example with a PR in the future. Maybe this would make sense as a separate PR though.

I haven't dug into the substance of your tests yet (hopefully someone more familiar with DDA will), but I verify that they pass!

@bgrant
Copy link
Collaborator

bgrant commented Jun 4, 2021

I'm glad you got the salesforce-cla check figured out!

Copy link
Collaborator

@briankeane briankeane left a comment

Choose a reason for hiding this comment

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

Just a comment about whether or not to keep the model unit tests -- you can take it or leave it. Very nice work @hmanalai!

@@ -0,0 +1,12 @@
COVERAGE_CMD = coverage run manage.py test --noinput && coverage xml && coverage report
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is great

Great suggestion @bgrant!

@@ -0,0 +1,21 @@
from django.test import TestCase
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure that this file is necessary -- for TDD it's great to have these, and they'd be great to have if we were developing the example app for use, but I'm not sure that they have much use as a demonstration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I was first writing the tests I wasn't sure what fields an instance of OAuthConsumer would have, and I thought maybe others would have the same question. That's why I left the model tests. But since we're going to have a documentation, I think we can take it out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nope that's a great answer!

@hmanalai hmanalai marked this pull request as ready for review June 7, 2021 15:33
@briankeane briankeane merged commit 4afd64d into salesforce:master Jun 7, 2021
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.

3 participants