Ensure high code coverage (now: 51.44%)

[aik099]

Just of curiosity I've decided to check code coverage on this repo. The number appeared to be as low as 51.44%. Maybe there wasn't high coverage initially or maybe not all added code was fully covered.

Either way it's time to raise it to prevent any errors in future.

Code Coverage: 51.44%

I also suggest to start using https://coveralls.io/ service, since it can show and track code coverage change with each PR and overall for a project.

P.S.
Code, that is uncovered by tests right now might be of 2 categories:

• forgot to be covered
• dead/unused code that needs to be removed

[stof]

For the code in Driver, it si not covered when running the Mink testsuite itself just because driver implementations have been moved to external repos, leaving only the base abstract classes. This is what hurts us a lot IMO (the other part being special exceptions but this can get covered)

[aik099]

As for Driver folder we can create driver mock and pass it to Session. Then just call Session methods to verify that calls pass through the Driver.

Not sure if ClassLoader and Compiler folders are needed at all (saw them in some other driver as well).

As for other folders, we can surely improve coverage.

[stof]

Compiling the library as a phar is useless IMO. a phar is useful for standalone tools, but Mink is not. It is a library used by your own code. Building a phar either needs to include the vendors, which will duplicate them when using them also in yoru project (potentially with different versions thus breaking things), or to exclude them, which makes the phar unusable. Thus, I don't think the phar is kept uptodate on the website. So my suggestion is to drop the phar distribution entirely.
And if we want to keep it, we should switch to using Box, like for Behat 3.0.

@everzet what is your choice ?

[aik099]

@stof, what is Box?

Does idea of having driver mock to improve coverage sounds good?

[stof]

@aik099 http://box-project.org/

[stof]

For the tests for CoreDriver, this would mean checking that all methods properly throw an UnsupportedDriverActionException, but it would require adding all older methods in the CoreDriver, even though they are implemented in all our existing drivers already (which was the reason to omit them when introducing the CoreDriver).
However, such test could have a benefit: it would enforce that all new methods added to the DriverInterface are implemented in CoreDriver (so that it does not break existing drivers extending CoreDriver)

[aik099]

Since Session relies on DriverInterface and not CoreDriver, then we can safely created mock just based on interface (that is first test case). And then create another test case for existing CoreDriver implementation, that will test that exceptions are thrown where they should.

[stof]

@aik099 Except that the only code needing coverage in the Driver namespace is CoreDriver...

But anyway, we have the same issue for CoreDriver than for other piece of code: many uncovered parts of the Element namespace are actually covered when running the driver tests. But as we moved drivers to separate repo, we lost all coverage reporting on the library.

[stof]

and covering the Element classes properly through unit tests only will be a huge pain because of the architecture of Mink (the driver is used inside everything)

[aik099]

and covering the Element classes properly through unit tests only will be a huge pain because of the architecture of Mink (the driver is used inside everything)

Not necessary. Driver is only used in session and everybody else uses session. Yes, getting larger coverage with integration tests is surely easier, than getting that through unit tests.

[stof]

@aik099 Look at the implementation of Element classes. They use the driver directly (getting it from the session)

[stof]

and we need (and have) the integration tests anyway for drivers. the issue is that drivers are in their own repo, so a coverage report will not include tests where they are run

[aik099]

@aik099 Look at the implementation of Element classes. They use the driver directly (getting it from the session)

I wasn't expecting that one. Reaching through objects is hard to mock during unit testing. Maybe we should rethink how driver is provided to the Elements. If session itself isn't needed by elements, then we should give only driver in constructor and not a session. Getting both driver and session to constructor of an Element class seems odd, but is DI/test friendly. All these kind of thoughts, if they ever will be implemented, surely would produce a large BC break anyway.

[stof]

@aik099 IMO, we should rethink the full Mink architecture. Even when limiting to the Session and the Driver, we can already see messy object graph, as the low level Driver has a setter to receive the higher level Session.
And the right fix for elements would probably be to avoid using the driver directly and rely only on the Session. But lets keep this issue focused. We should open a dedicated issue to discuss the changes we want to see in a Mink 2.0 version

[aik099]

So, for the drivers (I've opened an issue for each driver) we can increase code coverage without much effort. As for Mink this might be done only after major refactoring in future versions.

Seems reasonable?

[aik099]

Code coverage information is now being available on Scrutinizer CI. It doesn't prevent us from still using Converalls.Io as a dedicated coverage reporting service.

[stof]

well, from my experience on the Doctrine repos, where coveralls regularly decides to spam the PRs with comments about the coverage changes (which is a crappy integration with github anyway), while being explicitly disabled in the config (we don't like spammy comments), I'm not very much inclined about enabling it to be honest.
I opened scrutinizer-ci/scrutinizer#106 to request a code coverage overview on Scrutinizer (currently, we can only get it for a particular class or method)

[aik099]

where coveralls regularly decides to spam the PRs with comments about the coverage changes (which is a crappy integration with github anyway)

It wasn't doing so initially. Just recently saw it doing that. Doesn't like that.

I opened scrutinizer-ci/scrutinizer#106 to request a code coverage overview on Scrutinizer (currently, we can only get it for a particular class or method)

And I've posted my ideas there as well. If Scrutinizer can improve code coverage reporting then of course we don't need Coveralls.Io at all.

[stof]

We now have the coverage badge in the readme of all Mink-related projects

[stof]

once #456 is merged, the coverage report for Mink will look like this:

And reaching 100% coverage on the Session class is done by merging #453 as the only untested method is __clone (I left it untested on purpose while adding tests because of the pending PR).

It is looking pretty good.

The only remaining part without test is the Exception namespace, for the logic about building custom string representations of the exception

Regarding the other repos, here is the current coverage (according to Scrutinizer):

Repository	Coverage
Mink	92%
MinkSahiDriver	91%
MinkSeleniumDriver	90%
MinkSelenium2Driver	75%
MinkGoutteDriver	88%
MinkBrowserKitDriver	83%
MinkZombieDriver	77%
SahiClient	71%

MinkWUnitDriver does not have Scrutinizer enabled, but I don't think it is maintained anymore

[stof]

Updated list:

Repository	Coverage
Mink	90%
MinkSahiDriver	91%
MinkSeleniumDriver	96%
MinkSelenium2Driver	77%
MinkGoutteDriver	88%
MinkBrowserKitDriver	79%
MinkZombieDriver	78%
SahiClient	86%

The drop in MinkBrowserKitDriver is because the conversion of Httpfoundation responses into BrowserKit ones is not used anymore for BrowserKit 2.3+. The 2.0 branch (dropping the support of 2.2 and below) has 91% coverage

[stof]

Updated summary:

Repository	Coverage
Mink	90%
MinkSahiDriver	91%
MinkSeleniumDriver	96%
MinkSelenium2Driver	86%
MinkGoutteDriver	100%
MinkBrowserKitDriver	82%
MinkZombieDriver	86%
SahiClient	85%

The 2.0 branch of MinkBrowserKitDriver (dropping the support of 2.2 and below) has 97% coverage

[stof]

We are at 98% code coverage in Mink now.

[stof]

Closing this as we have a high-enough coverage in Mink itself. Other repos have their own issues to keep track of it