[provisioning api] fix failing testcase #19930
Conversation
| @@ -93,6 +93,8 @@ public function getUsers() { | |||
| $users = array_merge($users, $this->groupManager->displayNamesInGroup($group, $search)); | |||
| } | |||
|
|
|||
| asort($users); | |||
There was a problem hiding this comment.
uhh, thats a bit of a problem. The groupManager already sorts (or let's the user backend sort). This sorting might not be asort().
But well fine by me
There was a problem hiding this comment.
sorting a list of half a million users? no 👎
There was a problem hiding this comment.
sorting a list of half a million users? no 👎
Then tell me how to do proper pagination here.
There was a problem hiding this comment.
uhh, thats a bit of a problem. The groupManager already sorts (or let's the user backend sort). This sorting might not be asort().
But well fine by me
Well this is the same situation as with the sharee API... we get a list of users from several groups... So if we want to do pagination we need to properly sort first.
There was a problem hiding this comment.
Unified accounts table 🙈
There was a problem hiding this comment.
Yeah sure. But I would like not to have failing test cases until then.
There was a problem hiding this comment.
sort in the testcase 🙊
There was a problem hiding this comment.
Ok let me do a nasty hack on the tests then. (Note that once we have #19827 merged I can convert the provisioning app to pure unit tests and this should all go away...) Then we can leave the intergration tests to that nice job we have running already.
43c1080 to
64fdc11
Compare
64fdc11 to
3d281fc
Compare
|
Ok I modified the test. It is almost impossible to get the sort order right in the testcase to compare because databases sort is to dependant on the enviroment (the locale set has influence etc). So I just losend the test case here. Does mean we do not have strict tests for pagination of accessible users. |
| $this->assertEquals(['users' => array_slice($uids, 1, 2)], $result->getData()); | ||
|
|
||
| // Disable this test for now since sorting is not done the same on all backends | ||
| //$this->assertEquals(['users' => array_slice($uids, 1, 2)], $result->getData()); |
There was a problem hiding this comment.
you can compare arrays with phpunit
$this->assertEquals($expectedArray, $array, '', 0.0, true);There was a problem hiding this comment.
relevant part is the true for the parameter $canonicalize which allows array comare ignoring the order
There was a problem hiding this comment.
Still will not work since I need to slice the $expectedArray before passing it.
There was a problem hiding this comment.
why don't you just use sort on $result->getData() then?
You tried to sort it on the call. should be fine to sort it in the test the same way, no?
There was a problem hiding this comment.
No because I will have sliced after the sort.. assume 4 users:
['User1', 'User2', 'user4', 'User3']
Now assume the backend sorts like this:
['user4', 'User1', 'User2', 'User3']
Now the array is sliced and we return the middle 2 elements so:
['User1', 'User2']
However I don't know how the backend has sorted so I just sort in php which results in:
['User1', 'User2', 'User3', 'user4']
If I then slice I get:
['User2', 'User3']
So sorting what the call returns does not help me here. Because I don't know how the backend sorts my slice doesn't have to match the returned slice.
There was a problem hiding this comment.
I meanwhile question the value of this test .... we know pagination has issues ... no need to operate around it.
Anyway - let's get this merged to have a stable CI system
@rullzer please open an issue on pagination on the provisioning api
There was a problem hiding this comment.
|
👍 |
Since we have no control of how backends sort their list of users and we also don't want to sort yet another time the test now just checks if the correct number of elements is returned and if they are from the list of group members.
3d281fc to
9b64fa7
Compare
|
👍 |
[provisioning api] fix failing testcase
In order to make sure that slicing is done proplery the list of users should always be sorted.
This should fix the sometimes failing tests on Jenkins. exmaple
Patch is rather trivial.
CC: @MorrisJobke @PVince81 @nickvergessen @tomneedham