Page MenuHomekolab.org

Added infrastructure tests for Activesync/CalDav/WOPI/Roundcube & fixed the well-known redirect for caldav.
ClosedPublic

Authored by mollekopf on Mon, Jan 16, 2:34 PM.

Diff Detail

Repository
rK kolab
Lint
Lint Errors
SeverityLocationCodeMessage
Errorsrc/app/Backends/DAV.php:187PHPCS.E.PSR12.ControlStructures.ControlStructureSpacing.FirstExpressionLinePSR12.ControlStructures.ControlStructureSpacing.FirstExpressionLine
Errorsrc/app/Backends/DAV.php:306PHPCS.E.PSR12.Operators.OperatorSpacing.NoSpaceAfterPSR12.Operators.OperatorSpacing.NoSpaceAfter
Errorsrc/app/Backends/DAV.php:362PHPCS.E.PSR12.Operators.OperatorSpacing.NoSpaceAfterPSR12.Operators.OperatorSpacing.NoSpaceAfter
Errorsrc/tests/Infrastructure/ActivesyncTest.php:17phpstanInstantiated class Syncroton_Wbxml_Encoder not found.
Errorsrc/tests/Infrastructure/ActivesyncTest.php:20phpstanCall to method encode() on an unknown class Syncroton_Wbxml_Encoder.
Errorsrc/tests/Infrastructure/ActivesyncTest.php:30phpstanInstantiated class Syncroton_Wbxml_Decoder not found.
Errorsrc/tests/Infrastructure/ActivesyncTest.php:31phpstanCall to method decode() on an unknown class Syncroton_Wbxml_Decoder.
Errorsrc/tests/Infrastructure/DavTest.php:78phpstanUnreachable statement - code above always terminates.
Unit
No Unit Test Coverage
Build Status
Buildable 41363
Build 16586: arc lint + arc unit

Event Timeline

mollekopf requested review of this revision.Mon, Jan 16, 2:34 PM
mollekopf created this revision.
mollekopf updated this revision to Diff 11435.Mon, Jan 16, 5:02 PM

Include the roundcube test and fixed some lint issues

mollekopf added a reviewer: Restricted Project.Mon, Jan 23, 9:32 PM
machniak requested changes to this revision.Tue, Jan 24, 10:15 AM
machniak added a subscriber: machniak.
machniak added inline comments.
src/tests/Infrastructure/ActivesyncTest.php
41

getTestUser() is using fake queue. So, you'd have to execute the job(s) manually. Or using IMAP::createUser($user);

47

This should be configurable, please.

72

Use assertStringContainsString().

This revision now requires changes to proceed.Tue, Jan 24, 10:15 AM
mollekopf updated this revision to Diff 11468.Tue, Jan 24, 3:48 PM
mollekopf retitled this revision from Added infrastructure tests for our Activesync/CalDav/wopi infrastructure to Added infrastructure tests for Activesync/CalDav/WOPI/Roundcube & fixed the well-known redirect for caldav..
mollekopf edited the summary of this revision. (Show Details)

Test cleanup to work with test users.

Now includes an nginx fix because the commits were too cumbersome to untangle.

machniak requested changes to this revision.Wed, Jan 25, 10:22 AM
machniak added inline comments.
src/tests/Infrastructure/ActivesyncTest.php
48

I suppose this common code could be moved to getTestUser() (optional). Also, we should not use LDAP::createUser() if LDAP is disabled.

71

Should we use setUpBeforeClass() if we use tearDownAfterClass()? Also, we definitely want to remove the created user.

112

Debug code.

src/tests/Infrastructure/AutodiscoverTest.php
19

This too needs to be configurable. The chwala and webmail locations too. Then maybe instead of creating a separate config file for each "self" service we should put them somewhere together, e.g. in services.php or another file?

src/tests/Infrastructure/DavTest.php
53

Just use $this->deleteTestUser($this->user->email).

This revision now requires changes to proceed.Wed, Jan 25, 10:22 AM
mollekopf marked 3 inline comments as done.Thu, Jan 26, 9:08 AM
mollekopf added inline comments.
src/tests/Infrastructure/ActivesyncTest.php
71

They are static so I can't set member variables. That's why I ended up using setUp.

src/tests/Infrastructure/AutodiscoverTest.php
19

I understand the sentiment, but until we actually use the configs elsewhere I don't think it matters (which is why I didn't do it in the first place).

I like the idea of having a services.php file, and another variable in there also doesn't hurt, so I'll do that.

src/tests/Infrastructure/DavTest.php
53

tearDownAfterClass is a static function.

We can make deleteTest* and getTest* methods static.

mollekopf updated this revision to Diff 11510.Thu, Jan 26, 7:49 PM

Made uri's configurable, some cleanup, centralized test account setup and cleanup.

mollekopf planned changes to this revision.Fri, Jan 27, 12:04 AM
mollekopf added inline comments.
src/tests/TestCaseTrait.php
576

I thought this works, but it doesn't.

phpunit seems to sometimes reinstantiate the entire class between runs (member variables are lost), and beforeApplicationDestroyed is called on every tearDown().

So, phpunit makes it impossible to implement a setup first, run all tests, teardown last appraoch.

I'm going to stuff all tests into a single test function and be done with it.

mollekopf updated this revision to Diff 11516.Fri, Jan 27, 6:11 PM

This is what I've arrived at.

Unfortunately it's just not really possible I think to implement the user setup/teardown in a sensible fashion in phpunit without more intrusive changes.
The crux is that the laravel integration uses setUp/tearDown to setup and teardown the laravel framework, so outside of it we just can't really do anything (no db interaction anyways).
So we're stuck with lazy initialization, plus using a test and relying on the default ordering for teardown.
This now at least works.

In the process I also found an issue with the activesync test that would always return no folders, which I tracked to the initialization not being run after the account removal + creation,
because syncroton already knew the deviceid, it assumed the initial setup was done, but the metadata was empty of course (resulting in all folders being filtered including inbox).
Fixed by using a random deviceid.

machniak accepted this revision.Mon, Jan 30, 3:56 PM

I'd probably change a few things, but it's nothing that can't be done later.

This revision is now accepted and ready to land.Mon, Jan 30, 3:56 PM
mollekopf closed this revision.Wed, Feb 1, 9:54 AM

This has been shipped.