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

Integration tests #26

Merged
merged 23 commits into from
Nov 11, 2015
Merged

Integration tests #26

merged 23 commits into from
Nov 11, 2015

Conversation

blaix
Copy link
Contributor

@blaix blaix commented Nov 5, 2015

Closes #3

Also fixes a couple bugs found thanks to the tests

self.assertEqual(sub['id'], self.subscription['id'])

# Not working: https://github.com/yola/yolapy/issues/21
# def test_can_change_subscription_type(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be the closest thing I've seen to acceptable commented-out code... but I still don't like it.

Instead how about:

  • Commit the test as functional
  • Purge the test with a commit message linking the blocking issue
  • When (whoever) resolves the issue, they can revert the purge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that, but with one more step:

  • comment on the issues to restore the test in the linked commits

Copy link
Contributor

Choose a reason for hiding this comment

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

I was hoping that the github generated ref would be good enough but a comment is definitely better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the commented-out tests.

Will restore and flesh out when #20
is resolved.
Will restore after #19 is resolved.
Will restore after #21 is resolved.


from yolapy.services import Yola
from tests.test_integration.test_settings import url, auth
Copy link

Choose a reason for hiding this comment

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

I101 Imported names are in the wrong order. Should be auth, url

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like the type of helper that shouldn't live in the root of the source.

I do like the utility. Thoughts on moving it to __main__.py in yolapy and calling it via?:

python -m yolapy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't seem like the main program of yolapy. I don't think yolapy has a main program/purpose. This is a developer tool. I can't think of an alternative suggestion that isn't root though...

attrs = {
'id': 'WL_TEST-%s' % unique_id,
'name': 'TEST',
'parent_partner_id': 'WL_YOLA',
Copy link
Contributor

Choose a reason for hiding this comment

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

Using WL_YOLA here restricts which user/pass you can run the test settings.

Maybe that's okay?

This tripped me up when running. Suggesting either:

  • this line use the client's configured username
  • update the test sample to use username WL_YOLA

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed hard-coded partner ids in 86380d6

This will make it more flexible when changing user/pass in test
settings.

See #26 (comment)
This was a pattern that started repeating itself after I wrote the first
couple tests, but it became a noise word that was repeated in every
test.

Note: I typically dislike "test_[method_name]" names for unit tests, but
for integration tests like this, I think they are just fine.

See #26 (comment)
integration environment and edit the test settings::

$ cp tests/test_integration/test_settings.py.sample \
$ tests/test_integration/test_settings.py
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the prompt ($) on this line, as it's a continuation of the previous line

@beck
Copy link
Contributor

beck commented Nov 11, 2015

LGTM... after the small doc change:

👍


yola = Yola(url=url, auth=auth)

print 'The current directory has been added to the beginning of sys.path'
Copy link

Choose a reason for hiding this comment

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

T001 print statement found.

blaix added a commit that referenced this pull request Nov 11, 2015
@blaix blaix merged commit aa8956e into master Nov 11, 2015
@blaix blaix deleted the integration_tests branch November 11, 2015 19:25
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.

Add integration tests for service class
5 participants