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

Speedup #387

Closed
wants to merge 2 commits into from
Closed

Speedup #387

wants to merge 2 commits into from

Conversation

MajorCaiger
Copy link
Contributor

Hi @sebastianbergmann

Here are the speedup changes so far. Not sure if there is anything else that needs changing if the whitelist is to be mandatory. If you want to give it a quick check over and let me know if there is anything missing.

Thanks

Rob

@@ -7,10 +7,16 @@ All notable changes of the PHP_CodeCoverage 3.0 release series are documented in
### Changed

* It is now mandatory to configure a whitelist
* Merged [#384](https://github.com/sebastianbergmann/php-code-coverage/pull/384): Performance improvements
* It is now mandatory to configure a whitelist
* Merged [#384](https://github.com/sebastianbergmann/php-code-coverage/pull/384): Performance improvements

Choose a reason for hiding this comment

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

These duplications need to be cleaned up

@MajorCaiger
Copy link
Contributor Author

Hi @sebastianbergmann

I have now refactored the changes and re-introduced the processUncoveredFilesFromWhitelist. It now runs each test first to determine the covered lines/files, then it can use the coverage data to then determine uncovered files.

@sebastianbergmann
Copy link
Owner

Is this ready for me to look at, @MajorCaiger? Thanks!

@MajorCaiger
Copy link
Contributor Author

Yes please

@sebastianbergmann
Copy link
Owner

There are test failures:

PHPUnit 5.0.3 by Sebastian Bergmann and contributors.

.........FFFF.........................FFF.....                    46 / 46 (100%)

Time: 515 ms, Memory: 9.25Mb

There were 7 failures:

1) PHP_CodeCoverage_Report_CloverTest::testCloverForBankAccountTest
Failed asserting that format description matches text.
--- Expected
+++ Actual
@@ @@
 <?xml version="1.0" encoding="UTF-8"?>
 <coverage generated="1444162129">
   <project timestamp="1444162129" name="BankAccount">
     <file name="/usr/local/src/php-code-coverage/tests/_files/BankAccount.php">
       <class name="BankAccount" namespace="global">
-        <metrics methods="4" coveredmethods="3" conditionals="0" coveredconditionals="0" statements="10" coveredstatements="5" elements="14" coveredelements="8"/>
+        <metrics methods="4" coveredmethods="0" conditionals="0" coveredconditionals="0" statements="15" coveredstatements="5" elements="19" coveredelements="5"/>
       </class>
-      <line num="6" type="method" name="getBalance" crap="1" count="2"/>
+      <line num="6" type="method" name="getBalance" crap="1.12" count="2"/>
+      <line num="7" type="stmt" count="0"/>
       <line num="8" type="stmt" count="2"/>
       <line num="11" type="method" name="setBalance" crap="6" count="0"/>
+      <line num="12" type="stmt" count="0"/>
       <line num="13" type="stmt" count="0"/>
       <line num="14" type="stmt" count="0"/>
       <line num="15" type="stmt" count="0"/>
       <line num="16" type="stmt" count="0"/>
+      <line num="17" type="stmt" count="0"/>
       <line num="18" type="stmt" count="0"/>
-      <line num="20" type="method" name="depositMoney" crap="1" count="2"/>
+      <line num="20" type="method" name="depositMoney" crap="1.04" count="2"/>
+      <line num="21" type="stmt" count="0"/>
       <line num="22" type="stmt" count="2"/>
       <line num="24" type="stmt" count="1"/>
-      <line num="27" type="method" name="withdrawMoney" crap="1" count="2"/>
+      <line num="27" type="method" name="withdrawMoney" crap="1.04" count="2"/>
+      <line num="28" type="stmt" count="0"/>
       <line num="29" type="stmt" count="2"/>
       <line num="31" type="stmt" count="1"/>
-      <metrics loc="33" ncloc="33" classes="1" methods="4" coveredmethods="3" conditionals="0" coveredconditionals="0" statements="10" coveredstatements="5" elements="14" coveredelements="8"/>
+      <metrics loc="33" ncloc="33" classes="1" methods="4" coveredmethods="0" conditionals="0" coveredconditionals="0" statements="15" coveredstatements="5" elements="19" coveredelements="5"/>
     </file>
-    <metrics files="1" loc="33" ncloc="33" classes="1" methods="4" coveredmethods="3" conditionals="0" coveredconditionals="0" statements="10" coveredstatements="5" elements="14" coveredelements="8"/>
+    <metrics files="1" loc="33" ncloc="33" classes="1" methods="4" coveredmethods="0" conditionals="0" coveredconditionals="0" statements="15" coveredstatements="5" elements="19" coveredelements="5"/>
   </project>
 </coverage>

/usr/local/src/php-code-coverage/tests/PHP/CodeCoverage/Report/CloverTest.php:38

2) PHP_CodeCoverage_Report_CloverTest::testCloverForFileWithIgnoredLines
Failed asserting that format description matches text.
--- Expected
+++ Actual
@@ @@
       <line num="6" type="stmt" count="0"/>
-      <metrics loc="37" ncloc="25" classes="2" methods="0" coveredmethods="0" conditionals="0" coveredconditionals="0" statements="2" coveredstatements="1" elements="2" coveredelements="1"/>
+      <line num="29" type="stmt" count="0"/>
+      <line num="31" type="stmt" count="0"/>
+      <metrics loc="37" ncloc="25" classes="2" methods="0" coveredmethods="0" conditionals="0" coveredconditionals="0" statements="4" coveredstatements="1" elements="4" coveredelements="1"/>
     </file>
-    <metrics files="1" loc="37" ncloc="25" classes="2" methods="0" coveredmethods="0" conditionals="0" coveredconditionals="0" statements="2" coveredstatements="1" elements="2" coveredelements="1"/>
+    <metrics files="1" loc="37" ncloc="25" classes="2" methods="0" coveredmethods="0" conditionals="0" coveredconditionals="0" statements="4" coveredstatements="1" elements="4" coveredelements="1"/>
   </project>
 </coverage>

/usr/local/src/php-code-coverage/tests/PHP/CodeCoverage/Report/CloverTest.php:51

3) PHP_CodeCoverage_Report_CloverTest::testCloverForClassWithAnonymousFunction
Failed asserting that format description matches text.
--- Expected
+++ Actual
@@ @@
 <?xml version="1.0" encoding="UTF-8"?>
 <coverage generated="1444162129">
   <project timestamp="1444162129">
     <file name="/usr/local/src/php-code-coverage/tests/_files/source_with_class_and_anonymous_function.php">
       <class name="CoveredClassWithAnonymousFunctionInStaticMethod" namespace="global">
-        <metrics methods="2" coveredmethods="1" conditionals="0" coveredconditionals="0" statements="5" coveredstatements="4" elements="7" coveredelements="5"/>
+        <metrics methods="2" coveredmethods="1" conditionals="0" coveredconditionals="0" statements="6" coveredstatements="4" elements="8" coveredelements="5"/>
       </class>
-      <line num="5" type="method" name="runAnonymous" crap="1.04" count="1"/>
+      <line num="5" type="method" name="runAnonymous" crap="1.12" count="1"/>
+      <line num="6" type="stmt" count="0"/>

@@ @@
       <line num="18" type="stmt" count="1"/>
-      <metrics loc="19" ncloc="17" classes="1" methods="2" coveredmethods="1" conditionals="0" coveredconditionals="0" statements="8" coveredstatements="7" elements="10" coveredelements="8"/>
+      <metrics loc="19" ncloc="17" classes="1" methods="2" coveredmethods="1" conditionals="0" coveredconditionals="0" statements="9" coveredstatements="7" elements="11" coveredelements="8"/>
     </file>
-    <metrics files="1" loc="19" ncloc="17" classes="1" methods="2" coveredmethods="1" conditionals="0" coveredconditionals="0" statements="8" coveredstatements="7" elements="10" coveredelements="8"/>
+    <metrics files="1" loc="19" ncloc="17" classes="1" methods="2" coveredmethods="1" conditionals="0" coveredconditionals="0" statements="9" coveredstatements="7" elements="11" coveredelements="8"/>
   </project>
 </coverage>

/usr/local/src/php-code-coverage/tests/PHP/CodeCoverage/Report/CloverTest.php:64

4) PHP_CodeCoverage_Report_FactoryTest::testSomething
Failed asserting that 15 matches expected 10.

/usr/local/src/php-code-coverage/tests/PHP/CodeCoverage/Report/FactoryTest.php:42

5) PHP_CodeCoverageTest::testCollect
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
         32 => null
+        7 => Array ()
+        12 => Array ()
+        17 => Array ()
+        21 => Array ()
+        28 => Array ()
     )
 )

/usr/local/src/php-code-coverage/tests/PHP/CodeCoverageTest.php:269

6) PHP_CodeCoverageTest::testMerge
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
         32 => null
+        7 => Array ()
+        12 => Array ()
+        17 => Array ()
+        21 => Array ()
+        28 => Array ()
     )
 )

/usr/local/src/php-code-coverage/tests/PHP/CodeCoverageTest.php:300

7) PHP_CodeCoverageTest::testMerge2
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
         32 => null
+        7 => Array ()
+        12 => Array ()
+        17 => Array ()
+        21 => Array ()
+        28 => Array ()
     )
 )

/usr/local/src/php-code-coverage/tests/PHP/CodeCoverageTest.php:319

FAILURES!
Tests: 46, Assertions: 72, Failures: 7.

Generating code coverage report in Clover XML format ... done

Generating code coverage report in HTML format ... done

@MajorCaiger
Copy link
Contributor Author

Yikes, looks like I'm not quite there. When I tried to run the tests on master they were failing but they aren't now. I'll investigate.

@MajorCaiger MajorCaiger force-pushed the speedup branch 2 times, most recently from e2a7fe6 to bbd500b Compare October 19, 2015 07:40
@MajorCaiger
Copy link
Contributor Author

@sebastianbergmann

This merge request was started before version 3.0 was released and this was something you added when you tidied up the original merge request.

What would you like me to do about it?

@sebastianbergmann
Copy link
Owner

The blacklist is gone and the whitelist is now mandatory. This was not only done to make this speedup refactoring easier but most importantly to promote best practices.

@MajorCaiger
Copy link
Contributor Author

Shall I just remote the line from the change log? Or should I create a new change log? If so what version?

@sebastianbergmann
Copy link
Owner

I think it would be best if the speedup branch did not touch the ChangeLog-3.0.md file. I'll take care of it when I merge the branch into master.

Does it make sense for me to review the branch right now or are you still working on things?

@MajorCaiger
Copy link
Contributor Author

OK, I have removed the ChangeLog changes.

I think it is ready for review. All tests pass and I have run the unit tests with coverage on another project with and without these changes and got the same output.

@sebastianbergmann
Copy link
Owner

I tried the speedup branch with the test suite from https://github.com/sebastianbergmann/money/ and, among other differences, found the following:

master

master

speedup

speedup

The report generated by master is correct and the one generated by speedup is not.

@MajorCaiger
Copy link
Contributor Author

Hmm back to the drawing board then. I will take a look at the config from the money repo and see what I am missing.

Thanks

@MajorCaiger
Copy link
Contributor Author

Hi @sebastianbergmann

I have rebased and squashed the commits as they were getting a little messy.

I have altered the changes now so that all tests still pass, it shouldn't effect the outcomes of any other test suite (but I would be grateful if you could double check this). You now only get the speed improvements if you are running with processUncoveredFilesFromWhitelist turned on, otherwise there is no way of telling which files we can include_once prior to running the tests.

@Maks3w
Copy link
Contributor

Maks3w commented Oct 25, 2015

Shouldn't include a test for scenarios like the money repo?

Rob Caiger added 2 commits November 6, 2015 09:05
- Init data by running xdebug with 'unused' and 'dead code' flags on all whitelist files
- Run xdebug without flags for each test method
- Cache num lines check for each file, to prevent superflous checks
- (SB) Fix CS/WS issues
@sebastianbergmann
Copy link
Owner

I just rebased the branch to ease testing of its changes together with changes by @Maks3w.

if ($determineUnusedAndDead) {
xdebug_start_code_coverage(XDEBUG_CC_UNUSED | XDEBUG_CC_DEAD_CODE);
} else {
xdebug_start_code_coverage();
Copy link
Contributor

Choose a reason for hiding this comment

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

Path coverage will need all flags enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway path coverage in this point only facilitate ccn so it's ok.

@Maks3w
Copy link
Contributor

Maks3w commented Nov 6, 2015

I guess this is an alternative to sebastianbergmann/phpunit#1878

@sebastianbergmann
Copy link
Owner

Re: #387 (comment)

My plan is to merge the path coverage related changes before the speedup changes. We'll have to see then how the speedup changes need to be adapted.

@goetas
Copy link

goetas commented Dec 21, 2015

any news on this?

@paolomainardi
Copy link

Guys please keep us updated

@estahn
Copy link

estahn commented Mar 21, 2016

Any updates on this feature? Our tests which usually run in total of 3 minutes (without code coverage) run with code coverage in 1:30h.

I guess a more PHPUnit related question: Is there a way to execute code before and after the setUp method (e.g. subscribeEvent('onSetUpStart', ...), subscribeEvent('onSetUpFinish', ...)). Because the following actually reduces our test run time drastically:

    public function setUp()
    {
        xdebug_stop_code_coverage();
        ...
        xdebug_start_code_coverage();
    }

I would like to avoid adding the xdebug function calls everywhere.

@sebastianbergmann
Copy link
Owner

Closing because this has run out-of-sync with master. Please send a new pull request. Thanks!

@sebastianbergmann sebastianbergmann deleted the speedup branch May 4, 2016 05:43
@MPV
Copy link

MPV commented Jul 26, 2016

Has any one thought about trying this out again? @MajorCaiger? 😃

The speed improvements mentioned as part of this (in #384) sounded very promising. 👍

@MajorCaiger
Copy link
Contributor Author

Yeah, the changes got merged here :)

#433

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.

7 participants