Enable testing env for alternative home folder formats#26846
Conversation
|
@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @xoen and @butonic to be potential reviewers. |
|
Boom 💥 boom 💥 |
|
So far it looks like the tests do not properly clean up. Maybe the code that deletes the folders isn't actually deleting the users but trying to delete the matching folder in the data folder. In the case of the md5 home the name wouldn't match. Investigation continues... |
|
Found something: looks like the provisioning API doesn't manage to properly delete the user's home. |
|
Okay, here's the WTF: |
|
Fixed in ce3ae8b, I'll make a separate PR for this later. |
|
I've put the fix into a separate PR: #26848 |
|
Remaining failures: |
|
@SergioBertolinSG this is wrong: https://github.com/owncloud/core/blob/v9.1.3/build/integration/features/bootstrap/WebDav.php#L438 It assumes that $user is the name of the folder, but it could be something else like the md5. It looks like the provisioning API does not return the user's home folder which is bad, so the tests have no way to properly clear it. Maybe there's a workaround ? |
|
@SergioBertolinSG also don't put arbitrary files inside the "data" folder, that one should be kept only for the tested ownCloud instance. Use the "work" folder for that instead. Please have a look. There must be a way to test this differently. Use Webdav to check if the file was properly uploaded. |
|
PR to add a oc_preferences setting to override the home folder when using the database backend: #26851 |
|
This PR adds the "home" in the provisioning API response: #26853 |
|
Next steps:
Later on we'll also add a flag for enabling encryption. I have the feeling that we should revamp
Thoughts ? |
|
Perhaps we can just improve run.sh using functions and more arguments when calling it. |
Yes, that would also work. Note that in some cases preparing an env and setting behat filters can take several lines of code like the ones from encryption, so need to take this into account. |
Would that affect capabilities feature? it is enabled there in each scenario. |
|
@SergioBertolinSG it won't I think. The user backend is only enabled if you specify it in config.php. |
|
@SergioBertolinSG in a chat, @DeepDiver1975 pointed out that one could create a config file "test.config.php" and it would get integrated automatically. But this means that the integration tests need to fiddle within the ownCloud instance automatically without using APIs. I already don't like the fact that integration tests are writing stuff into the skeleton folder. (the reason I dislike is because I run Apache as another user than my dev user, so I need to Here's a PR to be able to override with a single oc_preferences #26867. |
There was a problem hiding this comment.
Since we'll likely have a lot of functions in the future for many scenarios, I suggest we start using prefixed.
For example: env_althome_enable, env_althome_clear. And in my other PR I could do the same with env_encryption_enable.
There was a problem hiding this comment.
I suggest to use prefixed for env variables as well, how about OC_TEST_ALT_STORAGE=1 (and in my other PR it would be OC_TEST_ENCRYPTION=1
There was a problem hiding this comment.
TEST_ALTHOME to be consistent with the functions
|
Last commit will bring conflicts to #26586. |
8b30182 to
5b8c2a1
Compare
|
Please rebase, the other PR was merged. But I can't explain why there is a conflict... |
5b8c2a1 to
9abc40c
Compare
|
Now @DeepDiver1975 has moved the integration tests to run only on stable/master branches, so need to rebase and adjust Jenkinsfile. But this also means that we won't see in this PR whether the tests pass. |
|
@SergioBertolinSG please rebase |
9abc40c to
774028d
Compare
|
Integration tests passed locally. |
PVince81
left a comment
There was a problem hiding this comment.
Please adjust Jenkinsfile then this is good to go 👍
I expect this to clash with the encryption integration tests but that's ok. We'll need to rebase that other PR to use a similar approach (functions) for setting up the encryption env.
| } | ||
| } | ||
|
|
||
| executeAndReport('build/integration/output/*.xml') { |
There was a problem hiding this comment.
Please move this into the isOnReleaseBranch() block
There was a problem hiding this comment.
It is already there, isn't?
Line 119 in 774028d
There was a problem hiding this comment.
Indeed... I misparsed the diff and saw a wrong indent.
Please ignore my comment
|
@SergioBertolinSG can you also rebase and delete the two first commits ? |
Sure. |
774028d to
fe5a9b7
Compare
|
Please remove the transifex commit |
fe5a9b7 to
2fc9276
Compare
|
The tests passed before, merging now |
| $OCC files_external:option $ID_STORAGE enable_sharing true | ||
|
|
||
| if test "$OC_TEST_ALT_HOME" = "1"; then | ||
| env_althome_enable |
There was a problem hiding this comment.
this cannot work, the proper name is env_alt_home_enable.
@SergioBertolinSG but you said you ran the tests locally and they passed ? 😦
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This is to see if integration tests pass when the user's home is md5'd.
A similar behavior would happen when selecting a home directory attribute with LDAP in which case the mounted home folder in ownCloud's virtual FS doesn't match the real filesystem.
There could be some bugs where
initMountPoint()was forgotten but it did work correctly with regular homes due to one to one name matching. This PR will tell us if there are other affected code paths...One known affected code path is #26820
Let's see how it goes...
Eventually I think we need an app or config.php option to be able to set/simulate this behavior when running unit tests and integratin tests. @DeepDiver1975 @owncloud/qa