[full-ci] Allow disabling the files_external app

[jvillafanez]

Description

Allow disabling the files_external app

Related Issue

https://github.com/owncloud/enterprise/issues/5036#issuecomment-1061743656

Motivation and Context

How Has This Been Tested?

• test environment:
• test case 1:
• test case 2:
• ...

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:
• Changelog item, see TEMPLATE

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[jvillafanez]

It seems the problem with the CI is that the files_external app is disabled when the phpstan runs, which causes the classes from the app not to get loaded. Enabling the app before hand should solve the problem, but we want to run the tests without the files_external app being enabled.

@phil-davis not sure how we want to go through this issue:

• Adding the failing files to the ignore list seems a bad idea.
• Forcing the app to be always enabled would defeat the purpose of this PR because we want to run the tests without the files_external app being active.
• Ignoring the whole files_external app might cause issues in the long run due to the lack of analysis.

I'm not sure if it's possible to use a different phpstan configuration depending on the state of the files_external app, or make it conditional somehow.

[jvillafanez]

------ --------------------------------------------------------------------- 
  Line   lib/private/Files/External/Service/LegacyStoragesService.php         
 ------ --------------------------------------------------------------------- 
  169    Call to static method decryptPasswords() on an unknown class         
         OC_Mount_Config.                                                     
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols   
 ------ --------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------- 
  Line   settings/Panels/Helper.php                                           
 ------ --------------------------------------------------------------------- 
  62     Call to static method dependencyMessage() on an unknown class        
         OC_Mount_Config.                                                     
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols   
 ------ --------------------------------------------------------------------- 

That's what I've found locally after running phpstan with files_external disabled. The second commit should fix those dependencies.

[ownclouders]

💥 Acceptance tests pipeline apiAuthOcs-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/34848/48/1

[phil-davis]

I fixed the phpstan problem. See the c ommit "Enable files_external for phpstan in drone"

Now there are fails in acceptance tests, like:
https://drone.owncloud.com/owncloud/core/34848/48/13

 Scenario: using OCS with an app password of a normal user                            # /drone/src/tests/acceptance/features/apiAuthOcs/ocsGETAuth.feature:243
    Given a new browser session for "Alice" has been started                           # AuthContext::aNewBrowserSessionForHasBeenStarted()
    And the user has generated a new app password named "my-client"                    # AuthContext::aNewAppPasswordHasBeenGenerated()
    When the user requests these endpoints with "GET" using the generated app password # AuthContext::userRequestsEndpointsUsingTheGeneratedAppPassword()
      | endpoint                                                    |
      | /ocs/v1.php/apps/files_external/api/v1/mounts               |
      | /ocs/v1.php/apps/files_sharing/api/v1/remote_shares         |
      | /ocs/v1.php/apps/files_sharing/api/v1/remote_shares/pending |
      | /ocs/v1.php/apps/files_sharing/api/v1/shares                |
      | /ocs/v1.php/config                                          |
      | /ocs/v1.php/privatedata/getattribute                        |
    Then the HTTP status code of responses on all endpoints should be "200"            # FeatureContext::theHTTPStatusCodeOfResponsesOnAllEndpointsShouldBe()
    And the OCS status code of responses on all endpoints should be "100"              # FeatureContext::theOCSStatusCodeOfResponsesOnAllEndpointsShouldBe()
      Expected same but found different ocs status codes of last requested responses.Found status codes: 999,100,100,100,100,100 (Exception)

Test scenarios like this are doing "quick" tests of various endpoints, all in the same scenario, for performance reasons. We need to separate all the test steps that are testing files_external and put them in separate scenarios. Then tag those scenarios, and skip the scenarios.

[phil-davis]

https://drone.owncloud.com/owncloud/core/34849/10/7

There were 195 errors

The unit tests will need to be looked through. If those 145 failing tests are all just tests that test something that includes files_external, then those tests will need minor adjustment so that they "skip" when files_external is not enabled.

[phil-davis]

I added a commit that separates out and skips some acceptance tests that relate to files_external.

It will take some work to sort out all the unit and acceptance tests so that they "understand" what to do when running with files_external disabled. Do we want to continue with this?

[phil-davis]

Acceptance tests pass - good.
Controlled skipping of PHP unit tests is needed. Do we want to do that?
(put in if files_external not enabled then $this->markTestSkipped() calls or something)

[jvillafanez]

I'm still checking unit tests but I might need more eyes on this.

Controlled skipping of PHP unit tests is needed. Do we want to do that?

The problem is what tests we want to skip. I'm checking some sharing tests, and those shouldn't be crashing even if the files_external app is disabled. Moreover, running those tests isolated (just those tests) doesn't give any problem.

So far, it seems that at some point, the AppConfig service is replace by a mock, it isn't restored. This seems to cause a lot of errors since the share API is considered to be disabled (due to the mock object) while the tests assume the share API is enabled.
Suspects are "tests/lib/App/ManagerTest.php", "tests/lib/AppConfigTest.php" and "tests/lib/AppTest.php".

[jvillafanez]

--- Expected
+++ Actual
@@ @@
         'canResetPassword' => true
         'resetPasswordLink' => null
         'alt_login' => Array ()
-        'rememberLoginAllowed' => false
+        'rememberLoginAllowed' => true
         'rememberLoginState' => 0
-        'strictLoginEnforced' => false
+        'strictLoginEnforced' => null
         'licenseMessage' => 'demo license valid'
     )
     'renderAs' => 'guest'

/drone/src/tests/Core/Controller/LoginControllerTest.php:262

PHP things: set the rememberLoginAllowed to true and the test passes even though the stricLoginEnforced was never set and was always null...

php > $a = ['a' => true, 'b' => false];
php > $b = ['a' => true, 'b' => null];
php > var_dump($a == $b);
bool(true)
php > $c = ['a' => false, 'b' => null];
php > var_dump($a == $c);
bool(false)

At least it seems we're getting closer. We still need to figure out a way to check whether the files_external app is enabled or not.
2e8faf4 assumes the files_external app is disabled, so the tests will fail otherwise. That's something we need to adjust.

[jvillafanez]

This seems to go through now. There are a couple of things to be discussed here:

• The way the checking of the availability of the files_external app doesn't seem good enough. However, I haven't found an easy way to check for the app without going through the appManager (which is what we're testing)
• Having conditional expectations in some of the tests doesn't look good. I'd prefer to have 2 or more tests with clear expectations. However, for this case, it seems that for the 2 tests we'd need, we'd have to skip always one of them
• Tagging and running different test suites, so for example, test1 would only run in the "files_external-is enabled" suite, while test2 would run in the "files_external-isn't-enabled" suite. This seems work I'm not sure we want to invest in now.

Other than that, I think the only thing left is to squash the commits a bit.

[phil-davis]

I will add CI pipelines that enable files_external and:

1. run the unit tests
2. run the acceptance tests tagged local_storage or files_external-app-required

That will help ensure that future regressions in the files_external code will be caught by CI.

[jvillafanez]

@phil-davis unless you have an idea for #39856 (comment) I think this is good to merge. I'm adding a changelog item now

[phil-davis]

@phil-davis unless you have an idea for #39856 (comment) I think this is good to merge. I'm adding a changelog item now

It looks good to me. Please invite some dev(s) to review and it could be merged. I can make a separate issue to put back some "full" testing for files_external in CI.

Note: for the next core oC10 release we will need to check exactly what happens for:

• new installs (is files_external enabled or not)
• upgrades of existing systems (make sure that files_external does not accidentally get disabled)

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

[IljaN]

LGTM but I guess @DeepDiver1975 should also have a look :)