-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sharing etag propagation tests #136
Conversation
Only for users, includes resharing and upload to several places
CC @icewind1991 |
Looking quickly at it, seems fine and seems like it's doing the job. |
This is expected. There is no actual file change to be synced for the owner.
Hmm, could be a bug. @icewind1991 is that the part where the owner needs to relogin ? (we haven't figured out whether it was a login hook or setup hook) |
I ran this against master and @icewind1991's new branch owncloud/core#20439 that reworks etag propagation. Both have the same issues. master:
the branch:
|
@PVince81 @jvillafanez @nickvergessen how long is that test execution time? THX |
it was 33 seconds on my laptop with SSD and smashdir in tmpfs. |
FS setup |
THX - candidate for jenkins ci job |
Unsharing now checks that the etag remains the same, so one of the points is fixed. I've added a bit more information about the checks so it should be easier to know what the test is doing The test_sharePropagation is passing in my environment, but the test_sharePropagationInside is failing
I'm checking the tests for errors, but I'm not finding anything weird in the tests |
Ok, it seems I forgot to add the workers 😶 Fixing.... |
@icewind1991 Could you confirm that the following cases (probably causing the above errors) have been taking into account?: Case 1:
My guess: etag propagates to U2, U3 and U1 but doesn't propagate through the reshare in U3 to U4. It should go U2 -> U1 -> U3 -> U4 Case 2:
The etag in /test for U2 remains the same but somehow the etag changes in / |
@jvillafanez @icewind1991 does it happen also if you manually test with the web UI / Webdav ? (with cadaver you can do "propget") |
Can't reproduce case 2 on master manually, maybe propagation was triggered by something unrelated |
Can't reproduce case 1 on master manually, all root etags change after uploading to test/sub as u2 |
For the first two errors, I was uploading the file to /test instead of /test/sub.... I'll recheck the last one. Anyway, I'm going to change the uploads from the owncloudcmd to pyocclient, I'm having some errors as if the files weren't being uploaded. |
No, don't change it. |
For the moment you can add a sleep(3); after creating the files, but that should be removed before the branch is merged. |
Please move the tests to lib/owncloud/ |
I've followed @nickvergessen to add a sleep before syncing, tests are almost there. What's left is the issue with the unshare. I think there might be something wrong with the test cleanup because first run works but then starts failing. |
|
||
step(3, 'Upload file') | ||
createfile(os.path.join(d, 'test', 'test.txt'), '1', count=1000, bs=10) | ||
time.sleep(3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
owncloud/client#4160 the issue has been fixed. So you can remove it when you use a git clone of the client or wait for the next daily build tomorrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather wait until 2.1 is out before removing the sleeps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well then we need to remember to remove it,
but I asked klaas for a beta2 soon, then we can remove it
owncloud/core#20439 Fixes the unshare tests |
owncloud/core#20439 + added sleep makes everything pass for me |
@PVince81 @nickvergessen merge? any failing tests seem to be bugs in master and there's already a pending fix |
I don't really like merging with the sleep, client is fixed with daily builds and I requested a new beta. But I guess we can still merge and just create a PR that removes all the sleeps and merge it, once beta2 is there |
Does smashbox run against the stable* versions ? If it does, then the test will fail these where the (non-backportable) fix is missing. |
Yes it does, but we can teach our selves to ignore them on version x.y |
👍 |
@jvillafanez please remove the |
Ready! |
👍 |
| 17 | verify etag is the same | verify etag is the same | verify propagation | | ||
+-------------+-------------------------+-------------------------+----------------------+ | ||
|
||
Remove the sleep(x) once https://github.com/owncloud/client/issues/4160 has a resolution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm
@nickvergessen merge ? |
check_users(4) | ||
|
||
reset_rundir() | ||
reset_server_log_file() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reset_rundir()
reset_server_log_file()
can be removed, smashbox is automatically doing that before each test since end of september
merge! |
Sharing etag propagation tests
Only for users, includes resharing and uploads to several places
Based on owncloud/core#14764 (comment)
I have to improve the error messages. Any ideas are welcomed.
There are a couple of potential issues detected (if the tests are correct):
@nickvergessen @DeepDiver1975 @PVince81