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

Prepare release 0.14.0 #473

Merged
merged 3 commits into from
Sep 11, 2020
Merged

Prepare release 0.14.0 #473

merged 3 commits into from
Sep 11, 2020

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Sep 11, 2020

@PVince81 PVince81 self-assigned this Sep 11, 2020
@phil-davis
Copy link
Contributor

cc70511 "Automated changelog update [skip ci]" appeared in master just now, causing GitHub to complain about this PR "This branch is out-of-date with the base branch"

@PVince81
Copy link
Contributor Author

we can merge anyway, after that there will be yet another changelog generation

@PVince81
Copy link
Contributor Author

Huh, weird... now the four tests I excluded in the other PR are passing...

@PVince81
Copy link
Contributor Author

I've rebased again and removed the 4 expected failures... I hope these are not random 🙈

@phil-davis
Copy link
Contributor

They are random. An example "fail" from a past run is:

     item "//d:response[2]/d:href" found with value "/remote.php/webdav/upload/file2.txt", expected to match regex pattern: "/remote\.php\/webdav\/upload\/file1.txt/"
        Failed asserting that '/remote.php/webdav/upload/file2.txt' matches PCRE pattern "/remote\.php\/webdav\/upload\/file1.txt/".

I think this issue: #471

    When user "Alice" gets the properties of folder "<folder_name>" with depth 1 using the WebDAV API                                           # WebDavPropertiesContext::userGetsThePropertiesOfFolderWithDepth()
    Then the value of the item "//d:response[1]/d:href" in the response to user "Alice" should match "/remote\.php\/<expected_href>\//"         # WebDavPropertiesContext::assertValueOfItemInResponseToUserRegExp()
    And the value of the item "//d:response[2]/d:href" in the response to user "Alice" should match "/remote\.php\/<expected_href>\/file1.txt/" # WebDavPropertiesContext::assertValueOfItemInResponseToUserRegExp()
    And the value of the item "//d:response[3]/d:href" in the response to user "Alice" should match "/remote\.php\/<expected_href>\/file2.txt/" # WebDavPropertiesContext::assertValueOfItemInResponseToUserRegExp()

The test steps assume a particular order of the folder and files in the PROPFIND response. That order is consistent in oC10 and with the OCIS "oc" storage. But this new OCIS "ocis" storage seems to send a more "random" order of files in the PROPFIND response.

The different order probably is acceptable - I doubt that the PROPFIND spec requires a particular sorted order of entries in the response.

So we need to make the test code more flexible.

How quickly to we need to do that!?

@PVince81
Copy link
Contributor Author

I heard that Golang was shuffling the order of hash maps when iterating, this might explain the random order.

@butonic should we sort the properties before returning ? or use another container ?

@PVince81
Copy link
Contributor Author

How quickly to we need to do that!?

No rush, but would be good to have this beginning of next week so we can tag "ocis".

The tests are green now but I'm unsure whether we should merge in this state...

@PVince81
Copy link
Contributor Author

@phil-davis the tests seem to be a bit too low level.

Instead of "Then the value of the item "//d:response[1]/d:href"" would likely better to say something like "there is an entry with href XYI in the response"

@phil-davis
Copy link
Contributor

We already merged in owncloud/ocis-reva master after a pass. And I am quite sure that when people create other PRs on master they will already sometimes get a pass and sometimes a fail. So merging this release branch will not make that any better or worse.

But yes, we should not pull this into owncloud/ocis and merge it until the test flexibility is sorted out.

@PVince81
Copy link
Contributor Author

converted to draft

as I understand, the test in question is just doing a PROPFIND, we certainly have better ways to check whether a folder contents ??

@phil-davis
Copy link
Contributor

phil-davis commented Sep 11, 2020

Test is improved in core PR owncloud/core#37902
Once that is confirmed and merged, I can update the core commit here so that CI uses the improved (more flexible) test.
The scenarios removed from expected-failures in this PR are correct - they should pass reliably with the new core test code.

@phil-davis
Copy link
Contributor

I have bumped the core commit id to get the new more-reliable core tests.
I will restart CI a couple of times to get confidence. Then this can be merged.

@phil-davis phil-davis marked this pull request as ready for review September 11, 2020 16:24
@phil-davis phil-davis merged commit 6a5c38d into master Sep 11, 2020
@delete-merged-branch delete-merged-branch bot deleted the release-0.14.0 branch September 11, 2020 16:42
@owncloud owncloud deleted a comment from update-docs bot Sep 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants