Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Unit tests for sharing are broken #151

Closed
DeepDiver1975 opened this Issue · 26 comments

7 participants

@DeepDiver1975

https://ci.tmit.eu/job/ownCloud-Server(master)/1265/testReport/

This is more or less a reminder for @schiesbn - we have been analyzing this last weekend in Berlin.

THX

@schiesbn schiesbn was assigned
@schiesbn
Collaborator

During the weekend we tragged it down to the point where the post_deleteUser hook where not emitted because no function was connected to the hook, even if it is defined in /lib/public/share.php.

There was no changes in the sharing code in between. More or less at the same time create user starts failing too because of some routing problems therefore we assumed that both problem could have the same roots.

@bartv2 could you have a look if this problem has something to do with you latest changes in the routing code? Thanks!

@DeepDiver1975

@bartv2 Can you please have a look at this?
Because of the tests are breaking on git master all pull request tests are breaking as well.
Thanks a lot!

@DeepDiver1975

@diederikdehaas you are right - the build has been thrown away :-(

@diederikdehaas
@fmms

To not loose them again here is the output of the failing test cases:

There were 4 failures:

1) Test_Share::testShareWithUser
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'test1.txt'
+    0 => 'test.txt'
+    1 => 'test1.txt'
 )

/var/lib/jenkins/jobs/ownCloud-Server(master)/workspace/tests/lib/share/share.php:259

2) Test_Share::testShareWithGroup
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => 'test.txt'
+    1 => 'test1.txt'
 )

/var/lib/jenkins/jobs/ownCloud-Server(master)/workspace/tests/lib/share/share.php:386

3) Test_News_MyTest::testTest
Failed asserting that false is true.

/var/lib/jenkins/jobs/ownCloud-Server(master)/workspace/tests/bootstrap.php:20
/var/lib/jenkins/jobs/ownCloud-Server(master)/workspace/apps2/news/tests/index.php:11

4) Test_Calendar_Calendars::testBasic
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
     'components' => 'VEVENT,VTODO,VJOURNAL'
+    'permissions' => 31
 )

/var/lib/jenkins/jobs/ownCloud-Server(master)/workspace/tests/bootstrap.php:12
/var/lib/jenkins/jobs/ownCloud-Server(master)/workspace/apps2/calendar/tests/calendar.php:33
@tanghus
Collaborator

4) Test_Calendar_Calendars::testBasic

Is probably to blame on me when I ported calendar app to the new sharing api. I'll have a look tomorrow.

@tanghus
Collaborator
@MTGap

No, permissions are a bitmask for a reason.

@DeepDiver1975

@MTGap from an API convenience point of view it is perfectly okay to provide combined masks.

@tanghus
Collaborator

I don't quite get why the calendar test fails. I've added another test in 57b3cfc689a55fc11aa73055909bb6779833d515 If that fails something is really wrong ;)

@tanghus
Collaborator
@MTGap

I don't see any reason for a developer using the Share API to need to know if all permissions are set or not. Adding/removing permissions is done by the sharing dropdown, so the only thing that a developer ever needs to check is if it is possible to edit/delete it.

I just see @tanghus wanting it for a test case, which isn't good enough of a reason.

@tanghus
Collaborator

@MTGap then:

const PERMISSION_ALL = PERMISSION_CREATE | PERMISSION_READ | PERMISSION_UPDATE | PERMISSION_DELETE | PERMISSION_SHARE;

;)

@tanghus
Collaborator

No, I need it for assigning permissions. Now I do it the "proper" way by giving all permissions to the owner in https://github.com/owncloud/apps/blob/master/calendar/lib/calendar.php#L50, but it would ease readability and be less prone for errors if I could just assign PERMISSION_ALL instead. If the API changes I wont have to change my code.

@tanghus
Collaborator

To elaborate on that: When querying for addressbooks/calendars I assign what equals to PERMISSION_ALL to the contacts owned by current user. That saves me a lot of client side code because I only have to check for the permissions, not for ownership.

so the only thing that a developer ever needs to check is if it is possible to edit/delete it.

Well, the developer has to check for permissions to edit/delete the object. Having to check both for ownership and for permissions kinda voids the CRUDS thingie we aim for ;)
If you're strongly against adding that const I'll accept that and keep it as is - it would just make things easier.

@diederikdehaas
@MTGap

@tanghus Now we're talking about using the sharing permissions outside of just shared items for your use case. This brings up if we should move the CRUDS permissions definitions outside of the Share API. No further comments here since it is unrelated to the issue, move to the mailing list if you want to continue.

@tanghus
Collaborator

I wrote:

Is probably to blame on me when I ported calendar app to the new sharing api. I'll have a look tomorrow.

Sorry, but I haven't had time to look at this. @georgehrke since you plan to change the Calendar sharing backend for OC 5, can we disable the array comparison test for now? I still haven't got autotest.sh working and spending hours on a test case that will likely be irrelevant soon doesn't feel very productive ;)

@bartv2
Collaborator

This issue is now solved, with commit 04aa029.

@bartv2 bartv2 closed this
@schiesbn
Collaborator

This might fix the test, but it does not fix the issue for the real user. The sharing app is still not loaded when a user gets deleted.

@schiesbn schiesbn reopened this
@bartv2 bartv2 closed this issue from a commit
@bartv2 bartv2 Create functions to install standard hooks
Also use these in tests that needs them
Fix #151
530f3f8
@bartv2 bartv2 closed this in 530f3f8
@schiesbn
Collaborator

sorry, I have to re-open this issue again. It is still not fixed for the real-world run. The problem is not where the hooks are defined. I think it is OK to define hooks only if the app is enabled and loaded. If the app is not loaded we should also avoid to trigger the hooks.

The problem was introduced by this commit bb136b9#settings/ajax/removeuser.php which removes "require_once '../../lib/base.php';" for many ajax calls, including settings/ajax/removeuser.php and which took care of loading all enabled apps.

A simple fix could be to bring this code back (but for some reasons this no longer works for me, i get an php error that he can't open base.php) or to add a OC_App::loadApps() instead, like proposed by my early merge request. But as mentioned by @blizzz it would be good to have this on a more generic place so that we don't have to call this on every file. But the problem with a (too) generic place is that we would always load all apps, even if we don't need it (e.g. webdav access).

@schiesbn schiesbn reopened this
@bartv2
Collaborator

@schiesbn no problem, github closed it earlier then i expected. Was expecting the close to happen on the merge. But back to the issue, this is still about the share hooks not being run on removeUser? With the pull the hooks should always be registered with loading base.php. Can you check that the are really there?

The general location to have the apps loaded would be in OC::handleRequest before the router->match call. This won't effect webdav access, but for other actions that don't need hooks or all apps loaded it would be to much. But at least it will solve the regression with loading all apps.

@schiesbn
Collaborator

But back to the issue, this is still about the share hooks not being run on removeUser? With the pull the hooks should always be registered with loading base.php. Can you check that the are really there?

Yes, but the problem is that base.php gets no longer loaded if you run removeUser, since the "require_once" was removed from the file. If we call base.php than it should work, also without moving the hooks from app.php to base.php because base.php takes care to load all necessary apps which than can register their hooks. I don't know why you removed the "required_once" statement from the ajax calls. But we need to find something which replaces it or ideally load it on a different more generic place which gets executed for every call. But than we have to take care to load only the apps we really need (e.g. for webdav calls we don't want to load the music player app but all logging apps and filesystem apps)

@bartv2
Collaborator

base.php is loaded, the url is .../index.php/settings/ajax/removeuser.php which loads index.php which loads lib/base.php which calls handleRequest which resolves the route (/settings/ajax/removeuser.php) and loads that file using actionInclude() function. So base.php is loaded and the sharing hooks are loaded there (in the pull).

The general location to have the apps loaded would be in OC::handleRequest before the router->match call.

but this still needs to be fixed

@schiesbn
Collaborator

sorry, my fault. I thought that your changes where already merged into master. Before I just tested master without checking the code. Now I tried your "share_hooks" branch. Looks good :+1:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.