Skip to content
This repository has been archived by the owner on Jan 18, 2021. It is now read-only.

Split old public API webdav tests from new public webdav tests #282

Closed
PVince81 opened this issue Jun 18, 2020 · 18 comments · Fixed by owncloud/core#38056
Closed

Split old public API webdav tests from new public webdav tests #282

PVince81 opened this issue Jun 18, 2020 · 18 comments · Fixed by owncloud/core#38056
Assignees
Labels

Comments

@PVince81
Copy link
Contributor

PVince81 commented Jun 18, 2020

AFAIK OCIS will only implement the new public webdav endpoint ("/public-webdav") as the old one "public.php/webdav" is deprecated.

@butonic please confirm

Many of the API tests are combining calls to old and new API within the same scenario, for example https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiSharePublicLink1/createPublicLinkShare.feature#L32

We need to split these cases in a way that we can disable old public webdav API tests for OCIS (with @skipOnOcis). Not sure if this needs a copy-paste of the whole test or whether we can disable "Examples" with tags.

This is needed so that we can enable new public webdav API tests for OCIS as part of #49

To find the related tests please grep for @issue-ocis-reva-282

@individual-it

@individual-it
Copy link
Member

probably the easiest would be to create a second file, one for old and one for new, tagging example tables is not possible (there is a PR for that in behat, but no progress on reviewing it Behat/Gherkin#161)
sadly that will increase test-execution time

@PVince81
Copy link
Contributor Author

Seems there are also tests that read the previews with publicpreview.php, I'll also tag those in owncloud/core#37552. They should use the "/public-files" with the file name suffix.

@PVince81
Copy link
Contributor Author

I saw in another file that the separation was done already by duplicating the scenarios.
I'll process a few already in my PR owncloud/core#37552

I managed to make a vim macro to replace all the blocks as they all have the same syntax.

@phil-davis
Copy link
Contributor

I saw in another file that the separation was done already by duplicating the scenarios.

For example, apiProvisioning-v1 and apiProvisioning-v2 test suites have the same scenarios, but the Background is different, either Given using OCS API version "1" or Given using OCS API version "2". That is a bit of "accidental" history.

If we have copy each scenario twice, then we have to. But it does create a maintenance issue when people adjust scenarios - making the matching updates is not always done 100%

@PVince81
Copy link
Contributor Author

I agree, this is not optimal. At least when the scenarios are in the same file it makes it easier to spot them. But looks ugly.

@PVince81
Copy link
Contributor Author

PVince81 commented Jun 19, 2020

another idea would be to have this driven with config/env vars: basically running all the existing/known suites using a given API version. this would make it possible also to run them in parallel without scenario duplication.
in case of rare result discrepancies, the tests could still be duplicated with related tags

@phil-davis
Copy link
Contributor

another idea would be to have this driven with config/env vars: basically running all the existing/known suites using a given API version. this would make it possible also to run them in parallel without scenario duplication.

Actually this should be quite easy to do, and I will just think of some Given step text in the style of:

    Given using the OCS API version defined externally

And that step can set the default OCS API version from some environment variable.

That will make it clear in the feature file Backgorund section that there is some "magic" happening.

And I can adjust .drone.star so it runs the relevant suites twice, with each of the versions 1 and 2.

@PVince81
Copy link
Contributor Author

@phil-davis sounds like a good idea.

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 7, 2020

remaining tests to split:

    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink1/accessToPublicLinkShare.feature:10
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink1/accessToPublicLinkShare.feature:20
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink1/accessToPublicLinkShare.feature:30
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink1/accessToPublicLinkShare.feature:43

and the following will likely require #11 to be solved first, still good to split soon:

    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:23
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:24
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:42
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:43
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:57
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:58
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:71
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:72
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:90
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:91
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:110
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:111
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:126
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:127
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:146
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:147
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:168
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink2/reShareAsPublicLink.feature:169

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 7, 2020

after splitting we'll likely discover either that more is working / can be enabled, or more bugs to raise or tag...

@phil-davis
Copy link
Contributor

remaining tests to split:

    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink1/accessToPublicLinkShare.feature:10
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink1/accessToPublicLinkShare.feature:20
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink1/accessToPublicLinkShare.feature:30
    /srv/www/htdocs/owncloud/tests/acceptance/features/apiSharePublicLink1/accessToPublicLinkShare.feature:43

These are single scenarios (not Scenario Outline with Examples table).

The step:

    When the public accesses the preview of file "testavatar.jpg" from the last shared public link using the sharing API

ends up in getPublicPreviewOfFile() which accesses /index.php/apps/files_sharing/ajax/publicpreview.php

I guess that is an "old" "private" endpoint, and there is a new official public endpoint for getting the preview of a file in a public link share.

@PVince81 when you say "to split", in this case do you mean "to add and engineer a new scenario that tests the official public endpoint", and so that new scenario could be run on both oC10 and OCIS?

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 9, 2020

Yeah, that private endpoint is very very old. Even in the OC time frame back then we switched to Webdav-based previews, so I'm not sure if we should still cover that legacy endpoint. Might need to check with clients before removing.

The Webdav based preview approach should work for both regular and public files IIRC. The approach is the one where you specify the Webdav path to a file and append some URL parameters to tell it to return the preview instead.

In the case of OCIS it is possible that the ocis-thumbnails service does not return previews yet, but that would be another issue to raise/tag once the splitting is done.

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 9, 2020

with splitting I mean keeping the old legacy and also add the new one, if that makes sense.

otherwise we could also simply rewrite that utility function to use the Webdav approach for retrieving previews.
still, we might need to have two variants of that one, one using "public.php/webdav" (aka old public endpoint) and one using "dav/public-files"

@individual-it
Copy link
Member

currently blocked by various ideas how to improve skipping tests etc. see https://jira.owncloud.com/browse/OCIS-217

@phil-davis
Copy link
Contributor

I can make a proof-of-concept for the ideas in https://jira.owncloud.com/browse/OCIS-217 - basically:

  • enhance the test runner so a "child" repo running the core test suite can specify a list of scenarios that are expected to fail.
  • for scenarios that are demonstrating different/wrong behavior in a "child" repo, put those in the "child" repo

The idea is to make the core scenarios be more of an independent test suite that "defines" the expected behavior of the API. And "child" repos can themselves easily adjust the scenarios that they skip, and make local "replacement" scenarios that demonstrate their current local "odd" behavior.

The aim is to reduce or eliminate the need for adjusting lots of scenario tags in core that need to be coordinated constantly with the state of the code in all the "child" repos.

@phil-davis
Copy link
Contributor

I am working on finishing implementation of owncloud/core#37717
When that is done, I hope it will be enough so that we do not need to split Scenario Outlines - let's see.

@individual-it
Copy link
Member

what is left to do here?

@phil-davis
Copy link
Contributor

Now we have expected-failures file, we have listed the scenarios that are expected to fail. So that is OK for now.

But it would be nice to not ever even run scenarios for old APIs that are not to be implemented on OCIS. At the moment, we do that by tagging them notToImplementOnOCIS. But when it is a Scenario Outline, we do not have the possibility to tag just 1 of the Scenario Examples.

That means:

  1. CI is wasting a bit of time running those scenarios over-and-over when they will always fail
  2. The expected-failures file will always have those scenarios, it will not become empty

If we could get Behat/Gherkin#161 merged and released in Behat/Gherkin thenwe could easily tag Scenario Examples. But I think that is never going to happen.

Thinking...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants