Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixed the assertions and test code cleanup #7

wants to merge 1 commit into


None yet
2 participants

dharkness commented Feb 7, 2012

I cleaned up the tests file so it will more accurately detect future breakages. Many of the string-comparison assertions were passing even though they weren't correctly checking the strings.

One test in particular, testGraphApiMethodWithOAuthSpecError(), was failing to detect such a bug. I corrected the test which causes a new test failure. The fix is trivial since the problem is a mismatched exception message, but I'll leave it to others to figure out whether the test or the code is incorrect. :)

I also renamed many of the stub classes so they all follow a single convention and cleaned up some formatting to follow the existing standard.

@dharkness dharkness Fixed all of the assertions:
- Type of assertion, e.g. assertContains() vs. assertTrue(strpos($haystack, $needle) != false)
- Order of arguments: expected before actual
- Logic, e.g. strpos() returns false when not found; not -1 or null
- Called statically since they are static methods
Converted to use @expectedException where possible
Removed unnecessary tearDown() and call to parent::setUp()
Changed support methods to be static where possible
Renamed stub/mock classes to follow a single standard
Fixed some incorrect formatting to match the standard

lacker commented Oct 10, 2013

Sorry for not getting to this before now. Unfortunately, since there are merge conflicts and this was a relatively minor thing in the first place, it's probably just better to let this drop. Sorry about that. Thanks for your contribution and if you do want to get this into master then please reopen another request.

@lacker lacker closed this Oct 10, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment