Add data from migrations table to report #64

sharidas · 2018-09-04T16:53:48Z

Add data from migrations table to the
report.

Signed-off-by: Sujith H sharidasan@owncloud.com

sharidas · 2018-09-04T16:54:29Z

Fixes #63

codecov-io · 2018-09-04T16:57:47Z

Codecov Report

Merging #64 into master will decrease coverage by 0.4%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##             master    #64      +/-   ##
==========================================
- Coverage       7.1%   6.7%   -0.41%     
- Complexity       52     53       +1     
==========================================
  Files             6      6              
  Lines           183    194      +11     
==========================================
  Hits             13     13              
- Misses          170    181      +11

Impacted Files	Coverage Δ	Complexity Δ
lib/AppInfo/Application.php	`0% <0%> (ø)`	`3 <0> (ø)`	⬇️
lib/Command/ConfigReport.php	`0% <0%> (ø)`	`3 <0> (ø)`	⬇️
lib/ReportDataCollector.php	`0% <0%> (ø)`	`37 <1> (+1)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c11fdcb...d1c1795. Read the comment docs.

patrickjahns · 2018-09-04T21:39:26Z

Would be great to expand the test-matrix now, to test configreport app in combination with several Database backends. ( mysql / postgres / oracle ) - so we can ensure that the report still works expected with the different supported databases

Database test improvments here:
#65

PVince81 · 2018-09-05T07:20:28Z

tests/unit/ReportDataCollectorTest.php

+			->willReturn($expectedResult);
+
+		$this->connection
+			->method('getQueryBuilder')


might have been better to put actual data into the oc_migrations table, but I guess there might be data there already due to setup

or check that the result contains some old known migrations from the real data ?

@PVince81 I captured the data from my dev machine and updated the test. Let me know if the updated test looks ok. For testing I used sqlite. Below is the console log:

➜ unit git:(add-oc-migration-data) ✗ /home/sujith/test/owncloud3/lib/composer/phpunit/phpunit/phpunit ReportDataCollectorTest.php Cannot load Xdebug - it was already loaded PHPUnit 5.7.27 by Sebastian Bergmann and contributors. Runtime: PHP 7.2.9-1+ubuntu18.04.1+deb.sury.org+1 with Xdebug 2.6.0 Configuration: /home/sujith/test/owncloud3/apps/configreport/tests/unit/phpunit.xml . 1 / 1 (100%) Time: 10.24 seconds, Memory: 44.00MB OK (1 test, 181 assertions) Generating code coverage report in Clover XML format ... done ➜ unit git:(add-oc-migration-data) ✗

Ouch hold on... I will ping again here. Just hold on for review. Until I fix this...

Ok, so the failure as of now is ..Makefile:41: recipe for target 'test-php-unit-dbg' failed https://drone.owncloud.com/owncloud/configreport/125/64.

@PVince81 Let me know the updated tests look ok.

PVince81 · 2018-09-05T07:21:35Z

this will also need a backport

PVince81 · 2018-09-05T08:28:31Z

wot

phpdbg -qrr -d memory_limit=4096M -d zend.enable_gc=0 "../../lib/composer/bin/phpunit" --configuration ./tests/unit/phpunit.xml
PHPUnit 5.7.27 by Sebastian Bergmann and contributors.

Runtime:       PHPDBG 7.1.20-1+ubuntu16.04.1+deb.sury.org+1
Configuration: /var/www/owncloud/apps/configreport/tests/unit/phpunit.xml

..Makefile:41: recipe for target 'test-php-unit-dbg' failed

https://drone.owncloud.com/owncloud/configreport/121/64

@patrickjahns

PVince81 · 2018-09-05T09:48:55Z

tests/unit/ReportDataCollectorTest.php

+			$this->assertArrayHasKey('app', $result);
+			$this->assertArrayHasKey('version', $result);
+			$values = \array_values($result);
+			$this->assertContains($values[0], ['core', 'dav', 'files_sharing', 'files_trashbin', 'notifications', 'dav', 'files_external', 'federatedfilesharing', 'customgroups']);


this will likely fail as we don't bundle "customgroups", etc

only pick the ones present in the core repository

I have updated the test, removed customgroups. I can't delete notifications, though. As the bundle owncloud-daily-master-qa.tar.bz2 has notifications app.

PVince81 · 2018-09-05T11:53:17Z

@sharidas you cannot list all migrations anyway, so only a few is enough.
As for apps, you also used assertContains() which is not an exact match but only a few, so removing "notifications" is ok.

sharidas · 2018-09-05T12:44:57Z

Updated the test. Removed notifications from the list. The iteration is done over an array of results, and if an app which is not part of the core app is found then no assertions are checked. If the apps are from core the assertions are checked. @PVince81

PVince81 · 2018-09-05T13:06:39Z

@patrickjahns should we ignore / disable to unblock this ?

+ if [ -z "true" ]; then make test-php-unit; fi
+ if [ "true" = "true" ]; then make test-php-unit-dbg; fi
phpdbg -qrr -d memory_limit=4096M -d zend.enable_gc=0 "../../lib/composer/bin/phpunit" --configuration ./tests/unit/phpunit.xml
PHPUnit 5.7.27 by Sebastian Bergmann and contributors.

Runtime:       PHPDBG 7.1.20-1+ubuntu16.04.1+deb.sury.org+1
Configuration: /var/www/owncloud/apps/configreport/tests/unit/phpunit.xml

make: *** [test-php-unit-dbg] Error 255
..Makefile:41: recipe for target 'test-php-unit-dbg' failed

not sure if related to this PR

PVince81

Code looks good 👍

sharidas · 2018-09-05T13:08:20Z

Code looks good

I am going to push the change for backport then. Thanks.

sharidas · 2018-09-05T13:32:14Z

Backport to stable10 branch #68

patrickjahns · 2018-09-05T17:44:50Z

not sure if related to this PR

Before this PR, the debugger would not cause segfaults and run out of memory. 4G should be limited to 2G anyway.

Neither the less - needs looking into, as we don't know what is causing the memory bloat when running the unit tests

patrickjahns · 2018-09-05T20:25:02Z

Removing the test and running it in drone results in no failure. So the test added here is the culprit.

Looking at the tests, this is not a simple unit test, it's trigger a large integration test.
Debugging further - the static call at https://github.com/owncloud/configreport/blob/master/lib/ReportDataCollector.php#L126 lets everything explode.

The whole reporter class should be refactored to become testable. Right now in the current state this will 💥 in our face

PVince81 · 2018-09-10T13:13:41Z

Any solutions in scope for 10.0.10 RC2 ?

Add data from migrations table to the report. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas · 2018-09-12T05:53:57Z

The pr is updated by skipping the test.

sharidas self-assigned this Sep 4, 2018

sharidas added the 3 - To Review label Sep 4, 2018

sharidas added this to the development milestone Sep 4, 2018

sharidas force-pushed the add-oc-migration-data branch from 4f6aebb to 5245a1d Compare September 4, 2018 17:24

patrickjahns mentioned this pull request Sep 4, 2018

Improve ci #65

Merged

sharidas force-pushed the add-oc-migration-data branch from 5245a1d to d7fb726 Compare September 5, 2018 07:10

sharidas changed the title ~~[WIP] Add data from migrations table to report~~ Add data from migrations table to report Sep 5, 2018

sharidas force-pushed the add-oc-migration-data branch from d7fb726 to 2eaaf8c Compare September 5, 2018 07:14

PVince81 reviewed Sep 5, 2018

View reviewed changes

PVince81 added the backport-request label Sep 5, 2018

sharidas force-pushed the add-oc-migration-data branch 4 times, most recently from 3011f3b to dc7db3d Compare September 5, 2018 08:58

PVince81 reviewed Sep 5, 2018

View reviewed changes

sharidas force-pushed the add-oc-migration-data branch 6 times, most recently from 7927e57 to bd242c4 Compare September 5, 2018 11:41

sharidas force-pushed the add-oc-migration-data branch 2 times, most recently from 8a03de7 to ac41883 Compare September 5, 2018 12:42

PVince81 approved these changes Sep 5, 2018

View reviewed changes

sharidas force-pushed the add-oc-migration-data branch from ac41883 to 22ad0db Compare September 5, 2018 13:17

sharidas mentioned this pull request Sep 5, 2018

[stable10] Backport of Add data from migrations table to report #68

Merged

sharidas removed the backport-request label Sep 5, 2018

sharidas force-pushed the add-oc-migration-data branch from 22ad0db to 937c025 Compare September 10, 2018 14:41

Add data from migrations table to report

d1c1795

Add data from migrations table to the report. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the add-oc-migration-data branch from 937c025 to d1c1795 Compare September 10, 2018 15:08

PVince81 merged commit b9f141c into master Sep 12, 2018

PVince81 deleted the add-oc-migration-data branch September 12, 2018 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add data from migrations table to report #64

Add data from migrations table to report #64

sharidas commented Sep 4, 2018

sharidas commented Sep 4, 2018

codecov-io commented Sep 4, 2018 •

edited

Loading

patrickjahns commented Sep 4, 2018 •

edited

Loading

PVince81 Sep 5, 2018

sharidas Sep 5, 2018

sharidas Sep 5, 2018

sharidas Sep 5, 2018

PVince81 commented Sep 5, 2018

PVince81 commented Sep 5, 2018

PVince81 Sep 5, 2018

sharidas Sep 5, 2018

PVince81 commented Sep 5, 2018

sharidas commented Sep 5, 2018 •

edited

Loading

PVince81 commented Sep 5, 2018

PVince81 left a comment

sharidas commented Sep 5, 2018

sharidas commented Sep 5, 2018

patrickjahns commented Sep 5, 2018 •

edited

Loading

patrickjahns commented Sep 5, 2018

PVince81 commented Sep 10, 2018

sharidas commented Sep 12, 2018

Add data from migrations table to report #64

Add data from migrations table to report #64

Conversation

sharidas commented Sep 4, 2018

sharidas commented Sep 4, 2018

codecov-io commented Sep 4, 2018 • edited Loading

Codecov Report

patrickjahns commented Sep 4, 2018 • edited Loading

PVince81 Sep 5, 2018

Choose a reason for hiding this comment

sharidas Sep 5, 2018

Choose a reason for hiding this comment

sharidas Sep 5, 2018

Choose a reason for hiding this comment

sharidas Sep 5, 2018

Choose a reason for hiding this comment

PVince81 commented Sep 5, 2018

PVince81 commented Sep 5, 2018

PVince81 Sep 5, 2018

Choose a reason for hiding this comment

sharidas Sep 5, 2018

Choose a reason for hiding this comment

PVince81 commented Sep 5, 2018

sharidas commented Sep 5, 2018 • edited Loading

PVince81 commented Sep 5, 2018

PVince81 left a comment

Choose a reason for hiding this comment

sharidas commented Sep 5, 2018

sharidas commented Sep 5, 2018

patrickjahns commented Sep 5, 2018 • edited Loading

patrickjahns commented Sep 5, 2018

PVince81 commented Sep 10, 2018

sharidas commented Sep 12, 2018

codecov-io commented Sep 4, 2018 •

edited

Loading

patrickjahns commented Sep 4, 2018 •

edited

Loading

sharidas commented Sep 5, 2018 •

edited

Loading

patrickjahns commented Sep 5, 2018 •

edited

Loading