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

implement __eq__ on base models + fix tests to be useful #54

Merged
merged 2 commits into from
Jul 24, 2015

Conversation

Rob-Johnson
Copy link
Contributor

the test that I submitted in #53 wasn't actually asserting anything - just that it didn't blow up like it did before. This makes sure that we're actually checking equality of the objects returned.

@solarkennedy
Copy link
Contributor

Can you merge master so we can actually see the results in travis?

Also, I don't really understand why the unit test wasn't asserting anything. Isn't this that the parse level, so simply listing deployment would have invoked the bug right?

Although I do appreciate the stronger assertion.

@Rob-Johnson
Copy link
Contributor Author

ok - I've merged master in so we should see the output in travis now.

True that we were originally just checking that we could parse the response properly, but we've now got stronger assertions about the objects that are produced rather than just 'it doesn't blow up'.

@solarkennedy
Copy link
Contributor

I'm trying to run the itests manually to verify your patch works on the itests

@solarkennedy
Copy link
Contributor

I ran the itests manually and this is fine.

solarkennedy added a commit that referenced this pull request Jul 24, 2015
implement __eq__ on base models + fix tests to be useful
@solarkennedy solarkennedy merged commit 89ba805 into thefactory:master Jul 24, 2015
@solarkennedy
Copy link
Contributor

Sorry @Rob-Johnson. While this does pass on the unit+itests here, it fails on our Yelp itest in our restart code.

I'm not going to revert this, as I know this functionality was broken before anyway, but there is still something not right.

I'll update our internal ticket with as much context as I can, and I'll try to figure it out for myself if possible. (but this is a tricky thing to catch)

If I really need to I'll copy over more of our itest coverage into here, specifically the failing test we have (serviceinit restart)

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