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

Fix casing of core test folder, bring back missing tests #26265

Merged
merged 6 commits into from
Mar 24, 2021

Conversation

PVince81
Copy link
Member

It seems Phpunit < 9 was case insensitive.
Fixed the phpunit config to target the correct name for the "Core" test
directory.

Fixes failing CI on PRs targetting master, like https://drone.nextcloud.com/nextcloud/server/3531/10/6

It seems Phpunit < 9 was case insensitive.
Fixed the phpunit config to target the correct name for the "Core" test
directory.

Signed-off-by: Vincent Petry <[email protected]>
@PVince81 PVince81 added bug 3. to review Waiting for reviews labels Mar 23, 2021
@PVince81 PVince81 added this to the Nextcloud 22 milestone Mar 23, 2021
@PVince81 PVince81 self-assigned this Mar 23, 2021
rullzer
rullzer previously approved these changes Mar 23, 2021
nickvergessen
nickvergessen previously approved these changes Mar 23, 2021
@PVince81
Copy link
Member Author

Oww, I wonder if those tests were just disabled/ignored:


1) Tests\Core\Command\Config\AppsDisableTest::testCommandInput with data set #0 (array('admin_audit'), 0, 'admin_audit 1.10.0 disabled')
--

@blizzz
Copy link
Member

blizzz commented Mar 23, 2021

There were 27 errors:

  1. Tests\Core\Command\Preview\RepairTest::testEmptyExecute with data set #0 (array(), 'All previews are already migrated.')
    ArgumentCountError: Too few arguments to function OC\Core\Command\Preview\Repair::__construct(), 4 passed in /drone/src/tests/Core/Command/Preview/RepairTest.php on line 46 and exactly 5 expected

ChristophWurst
ChristophWurst previously approved these changes Mar 24, 2021
@PVince81
Copy link
Member Author

I've fixed more tests from the spotted reports, let's see...

Remove "ocs-provider" test folder reference as it doesn't exist any
more.
Added back "Test" test subdir and fixed the tests inside.

Signed-off-by: Vincent Petry <[email protected]>
@PVince81
Copy link
Member Author

And, fixed even more tests.

Seems there was also a subdir "ocs-provider" that was referenced but didn't exist any more.
While looking, I found another forgotten folder "test/Test" containing some Owncloud-DB related repair steps. I've fixed and enabled those as well now.

@PVince81
Copy link
Member Author

seems the can of worms is even deeper:

1) Test\APITest::testIsV2 with data set #0 ('/master/ocs/v2.php', true)
Error: Call to undefined method Test\APITest::getAnnotations()

/drone/src/tests/lib/TestCase.php:468
/drone/src/tests/lib/TestCase.php:150

definitely related to the implicit Phpunit version bump in the PHP 8 pipeline.

for now I'd say let's pin it to Phpunit 8.x for PHP 8.
at least the version bump has helped us identify tests that were disabled, so at least that's a win...

@PVince81
Copy link
Member Author

hmm, as far as I can see phpunit 9 was added 3 months ago and it used to work: https://github.com/nextcloud/docker-ci/blame/d1ad17acfac53762812d1421376212d99eda7dde/php8.0/Dockerfile#L13

since we didn't pin to a minor version, it seems that there were API changes in PHPUnit in 9.5: https://www.drupal.org/project/drupal/issues/3186443

@PVince81
Copy link
Member Author

PR for the docker-ci to set the PHPUnit version to 9.4.4: nextcloud/docker-ci#274

@PVince81
Copy link
Member Author

waiting for php8.0-2 to appear in https://hub.docker.com/r/nextcloudci/php8.0/tags?page=1&ordering=last_updated then we can restart CI

@PVince81
Copy link
Member Author

restarted CI as the new image has been published

@PVince81
Copy link
Member Author

one of the "new" tests that got reenabled fail on Oracle:

1) Test\Repair\Owncloud\UpdateLanguageCodesTest::testRun
Asserts that the entries are the ones from the test data set
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
     0 => Array &1 (
+        'userid' => 'user3'
+        'configvalue' => 'fi'
+    )
+    1 => Array &2 (
         'userid' => 'user1'
         'configvalue' => 'fi_FI'
     )
-    1 => Array &2 (
-        'userid' => 'user2'
-        'configvalue' => 'de'
-    )
     2 => Array &3 (
-        'userid' => 'user3'
-        'configvalue' => 'fi'
+        'userid' => 'user4'
+        'configvalue' => 'ja'
     )
     3 => Array &4 (
-        'userid' => 'user4'
-        'configvalue' => 'ja'
+        'userid' => 'user2'
+        'configvalue' => 'de'
     )
     4 => Array &5 (
         'userid' => 'user5'

/home/runner/work/server/server/tests/Test/Repair/Owncloud/UpdateLanguageCodesTest.php:94

Fixes issue with Oracle by enforcing the order of the results to check.

Signed-off-by: Vincent Petry <[email protected]>
@PVince81
Copy link
Member Author

all the other DB tests passed, so we're good for php8 and the new tests.

I've pushed a fix for Oracle to enforce the check order.

Now unless acceptance tests decide to randomly fail, I expected full green

@PVince81
Copy link
Member Author

Note: as observed on another PR https://drone.nextcloud.com/nextcloud/server/3560, it seems CI only needed the phpunit docker-ci fix to work again.

Still, we should move forward with this PR here as it reenables and fixes tests that were missing previously.

@PVince81 PVince81 changed the title Fix casing of core test folder Fix casing of core test folder, bring back missing tests Mar 24, 2021
@PVince81
Copy link
Member Author

strange, it's the second time that /drone/src/tests/acceptance/features/header.feature:33 fails

# ContactsMenuContext::iSeeThatTheContactsMenuIsShown()
    And I see that the contact "user0" in the Contacts menu is not shown                                                      # ContactsMenuContext::iSeeThatTheContactInTheContactsMenuIsNotShown()
      Failed asserting that true is false.
    And I see that the contact "admin" in the Contacts menu is not shown                                                      # ContactsMenuContext::iSeeThatTheContactInTheContactsMenuIsNotShown()

@PVince81 PVince81 dismissed stale reviews from ChristophWurst, nickvergessen, and rullzer March 24, 2021 11:04

a lot has changed

@PVince81
Copy link
Member Author

it's green now, please re-review as a lot has changed

important to note is that now we have more tests enabled than before.
I'm curious to see if it will survive backporting

@nickvergessen nickvergessen merged commit 4406092 into master Mar 24, 2021
@nickvergessen nickvergessen deleted the bugfix/noid/fix-php8-setup-failures branch March 24, 2021 12:25
@PVince81
Copy link
Member Author

/backport to stable21

@PVince81
Copy link
Member Author

/backport to stable20

@PVince81
Copy link
Member Author

/backport to stable19

@PVince81
Copy link
Member Author

will it backport ? 🥁

@backportbot-nextcloud
Copy link

The backport to stable19 failed. Please do this backport manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants