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

Add Covers PHPUnit Annotation to Controller Baked Classes #16

Open
justinyost opened this issue Jul 29, 2015 · 5 comments
Open

Add Covers PHPUnit Annotation to Controller Baked Classes #16

justinyost opened this issue Jul 29, 2015 · 5 comments

Comments

@justinyost
Copy link
Contributor

More information: https://phpunit.de/manual/current/en/appendixes.annotations.html#appendixes.annotations.covers.tables.annotations

Note @covers requires the full namespaced path, like so @covers \App\Controller\Anon\CampSessionsController::index

@justinyost justinyost changed the title Add Default Covers Statements to Controller Baked Test Classes Add Covers PHPUnit Annotation to Controller Baked Classes Jul 29, 2015
@beporter
Copy link
Contributor

I'm on the fence about this without going the extra step and having explicit unit vs integration test suites for controllers. As much as I think there are problems with "defaulting" to integration tests, it's what Cake 3 expects, and despite appearances I don't really want to stray too far from that.

In addition, the @covers annotation itself is a little dangerous, especially when changing method names. Forgetting to update the annotation means that you get an unexplained drop in coverage. I guess this is mitigated pretty well by ensuring a minimum coverage percentage, but it can be difficult to debug why something you think should be getting covered isn't until you're in the habit of checking for the annotation. This is something we'll adapt to if it's "normal", but it's worth pointing out.

Another (possible) complication is that this gets really sticky when you get into prefixed/namespaced classes. Take a look at what we had to do in Eyesore because of the way we tested each namespaced method using a shared test case using each user role as a @dataProvider: https://github.com/loadsys/Eyesore-ORB/pull/25/files#diff-e7e26aeee7bd3d540cc16d44d4affb14R85

@justinyost
Copy link
Contributor Author

Yeah I just updated the comment to include that it would require the full namespaced path.

@justinyost
Copy link
Contributor Author

The big thing this is that trying to get coverage in a project means I have to fight with the integration tests doing coverage because the coverage is lying to me.

@beporter
Copy link
Contributor

Yeah, that's a valid point. I thought I had opened a core or bake issue about that, but I can't seem to find it now.

@justinyost
Copy link
Contributor Author

Regarding the changing method names, that makes sense for things like Tables, Components, etc but since Controller method names are (generally) the url being hit they rarely change at least what we've generally observed. Adding it only to the Controller might be a way of mitigating this need to change the annotation when updating a method name.

@justinyost justinyost modified the milestone: Version 1.0 Sep 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants