Skip to content

Adding more trashbin scenarios#28111

Merged
PVince81 merged 16 commits into
masterfrom
inttest-adding-trashbin-scenarios
Jun 30, 2017
Merged

Adding more trashbin scenarios#28111
PVince81 merged 16 commits into
masterfrom
inttest-adding-trashbin-scenarios

Conversation

@SergioBertolinSG

Copy link
Copy Markdown
Contributor

Something is failing in the second scenario. The listed elements of the trashbin are less than expected and the file the case is looking for is not found.

@SergioBertolinSG

Copy link
Copy Markdown
Contributor Author

The requests to index.php/apps/files_trashbin/ajax/list.php?dir=%2Fare not returning all the elements in the trashbin. It is having troubles with the files in subfolders.
Only happening when the files has the same name. @PVince81

When User "user1" deletes folder "/shared"
Then as "user1" the folder with original path "/shared" does not exist in trash

Scenario: deleting a file in a recieved folder doesn't moves it to trashbin

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few "recieved" - here and further down.
s/recieved/received
Isn't English a great language ;)

@SergioBertolinSG

Copy link
Copy Markdown
Contributor Author

Pending to be done: fixes for the two failing cases. Check if the whole suite pass after last changes.

@SergioBertolinSG SergioBertolinSG force-pushed the inttest-adding-trashbin-scenarios branch from a91f271 to ed84700 Compare June 16, 2017 11:21
@felixboehm

Copy link
Copy Markdown
Contributor

If your suite contains tests, which point out bugs, is this a blocker for merging the tests?
Suggestion: Can you open an PR for each, containing the failing testcase, so that fixing and test come together in that PR?

@phil-davis

phil-davis commented Jun 16, 2017

Copy link
Copy Markdown
Contributor

The other option, if fixes are going to be some time away, is to tag the scenarios that fail with @Skip so that they are skipped in "normal" CI runs. Then in the PR that fixes the bug, also remove the @Skip tag.
(and have the script that runs tests default to ~@Skip to not run scenarios tagged to be skipped.

@SergioBertolinSG

Copy link
Copy Markdown
Contributor Author

Failing cases in jenkins:

trashbin-new-endpoint.deleting a file in a received folder it is moved to the trashbin
trashbin-new-endpoint.trashbin can store two files with same name but different origins
trashbin-old-endpoint.deleting a file in a received folder it is moved to the trashbin
trashbin-old-endpoint.trashbin can store two files with same name but different origins
trashbin-old-endpoint.Deleting a folder into external storage moves it to the trashbin
trashbin-old-endpoint.Deleting a file into external storage moves it to the trashbin and can be restored

Pending to implement the skip label to fix this cases in a different PR.

@PVince81

Copy link
Copy Markdown
Contributor

are these cases reproducible manually ? AFAIK at least storing two files with different origins should still work.

@SergioBertolinSG

Copy link
Copy Markdown
Contributor Author

Regarding "deleting a file in a received folder it is moved to the trashbin", yes I opened it here, the problem is a missing parameter called 'extraData':

#28121

Regarding "trashbin can store two files with same name but different origins", I cannot reproduce it manually, but in the tests it always fails.

trashbin-old-endpoint.Deleting a folder into external storage moves it to the trashbin

This one is a test problem, I can fix it.

trashbin-old-endpoint.Deleting a file into external storage moves it to the trashbin and can be restored

This one has the same problem as the previous one, fixable in tests, but maybe the restoration part is affected also by the missing 'extraData' parameter bug.

@PVince81

Copy link
Copy Markdown
Contributor

if manual reproduction doesn't work but automated test fails, then maybe the tests aren't doing exactly the same, or it's not using the same APIs.

@SergioBertolinSG

SergioBertolinSG commented Jun 22, 2017

Copy link
Copy Markdown
Contributor Author

Now the tests:

trashbin-new-endpoint.deleting a file in a received folder it is moved to the trashbin
trashbin-new-endpoint.trashbin can store two files with same name but different origins
trashbin-old-endpoint.deleting a file in a received folder it is moved to the trashbin
trashbin-old-endpoint.trashbin can store two files with same name but different origins

Are skipped. To be removed the "@Skip" label to run them when fixing the actual core issues.

@SergioBertolinSG SergioBertolinSG changed the title [WIP] Adding more trashbin scenarios Adding more trashbin scenarios Jun 22, 2017
@PVince81

Copy link
Copy Markdown
Contributor

trashbin-new-endpoint.deleting a file in a received folder it is moved to the trashbin

Is this reproducible manually ? Last time I tested this manually when working on this part all worked fine.

@PVince81

Copy link
Copy Markdown
Contributor

Actually both the owner trashbin and recipient trashbin should receive the file.

@SergioBertolinSG

Copy link
Copy Markdown
Contributor Author

@PVince81 the problem is the lack of extraData field. Which makes this step to fail:
Then as "user1" the file with original path "/shared/shared_file.txt" exists in trash

The file is in the trashbin, but if it is restored it will go to the root folder instead to the original path. #28121

@PVince81

Copy link
Copy Markdown
Contributor

ok

@SergioBertolinSG SergioBertolinSG force-pushed the inttest-adding-trashbin-scenarios branch 2 times, most recently from 4f1d176 to 3331362 Compare June 23, 2017 12:52
@PVince81

Copy link
Copy Markdown
Contributor

Here is the fix for restoring into the wrong folder: #28196

@PVince81

Copy link
Copy Markdown
Contributor

@SergioBertolinSG SergioBertolinSG force-pushed the inttest-adding-trashbin-scenarios branch 2 times, most recently from 1a3ac4a to d31607b Compare June 27, 2017 14:21
@SergioBertolinSG SergioBertolinSG force-pushed the inttest-adding-trashbin-scenarios branch from d31607b to 1c9915e Compare June 28, 2017 15:25
@SergioBertolinSG SergioBertolinSG force-pushed the inttest-adding-trashbin-scenarios branch from 1c9915e to aa1fbcc Compare June 29, 2017 08:01
| /textfile3.txt |
| /textfile4.txt |

@skip

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we still have skipped tests ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that scenario is not related with the extraData fix.

It doesn't fail manually, but in the test it always fail. I recommend to merge this and take a look when you want. The test doesn't seem wrong, looks like a hidden bug not reproducible using the ui.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hidden bug

I hate these

@PVince81 PVince81 merged commit 8b06e37 into master Jun 30, 2017
@PVince81 PVince81 deleted the inttest-adding-trashbin-scenarios branch June 30, 2017 09:19
@PVince81

Copy link
Copy Markdown
Contributor

backport?

@SergioBertolinSG

Copy link
Copy Markdown
Contributor Author

Ok, to stable10 and others? or just stable10?

@PVince81

Copy link
Copy Markdown
Contributor

as far as possible I'd say to guarantee coverage on older versions too.

@lock

lock Bot commented Aug 3, 2019

Copy link
Copy Markdown

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.

@lock lock Bot locked as resolved and limited conversation to collaborators Aug 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants