-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[stable10] tests async operations #32594
Conversation
33fe2dc
to
ad21a2d
Compare
$stream = false; | ||
if ($type === "asynchronously") { | ||
$headers['OC-LazyOps'] = 'true'; | ||
if ($this->httpRequestTimeout > 0) { |
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.
does makeDavRequest
read this attribute ?
having the server slow down could make some test wait forever when we don't want to wait
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.
the slow-down need to be initialized by @Given the MOVE dav requests are slowed down by 10 seconds
see slowdownDavRequests()
whatever the setting was before the test-run its set back to it by https://github.com/owncloud/core/pull/32594/files#diff-76a3f5f23003d99750d877daa045562aR2478
in these lines here we are only guessing that we need to set $stream=true
. we need it because the server does not close the connection while its slowed down by the testing app but we need already some information from the header to do the next step.
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.
Looks good, apart from one concern: is there a situation where the tests would needlessly wait for the server ? Is the client able to kill the connection if it's stuck for expected X seconds ?
If someone sets the setting needlessly the server would wait, but if the correct test steps are used it should not be a problem. |
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.
Looks good.
See minor comment about utility method for config php settings
)['stdOut']; | ||
$this->oldAsyncSetting = \trim($oldAsyncSetting); | ||
} | ||
$this->runOcc( |
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.
did we not have utility methods yet for setting config.php settings ? perhaps we only had for oc_appconfig
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.
there is no one yet, but definitely we should have one. proposing to make a new PR for that
issue opened #32666
should we have it in the coming sprint?
if ($this->oldAsyncSetting === "") { | ||
SetupHelper::runOcc(['config:system:delete', 'dav.enable.async']); | ||
} elseif ($this->oldAsyncSetting !== null) { | ||
SetupHelper::runOcc( |
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.
and here's the occ again, might be useful to have this in a utility method ? (if not too complex to add as it needs to be accessible from every context)
a wild conflict appears |
ad21a2d
to
1b5a2f0
Compare
Codecov Report
@@ Coverage Diff @@
## stable10 #32594 +/- ##
===========================================
Coverage ? 62.92%
Complexity ? 18853
===========================================
Files ? 1235
Lines ? 73911
Branches ? 1282
===========================================
Hits ? 46511
Misses ? 27020
Partials ? 380
Continue to review full report at Codecov.
|
@PVince81 just killed that wild beast |
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.
👍
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.
It looks good. The little PHPdoc things could be fixed in a next PR, unless you can wait ffor another CI run before merging.
@@ -1753,17 +1873,23 @@ public function userUploadsTheFollowingChunksUsingNewChunking($user, $file, Tabl | |||
* [0] the chunk number | |||
* [1] data content of the chunk | |||
* Chunks may be numbered out-of-order if desired. | |||
* | |||
* @package bool $async use asynchronous MOVE at the end or not |
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.
package or param ?
@@ -2274,4 +2461,40 @@ public function userRestoresVersionIndexOfFile($user, $versionIndex, $path) { | |||
['Destination' => $this->makeSabrePath($user, $path)] | |||
); | |||
} | |||
|
|||
/** | |||
* reset settings if there were set in the scenario |
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.
if they were set
1b5a2f0
to
f4beb74
Compare
@phil-davis addressed your comments |
👍 |
Drone network errors - restarted. |
restarted again |
@PVince81 now it is ready for merge. |
Description
tests for #32414 (moving files using async operation)
one test depends on changed in the testing app owncloud/testing#21
Related Issue
Motivation and Context
see issue
How Has This Been Tested?
🤖
Types of changes
Checklist:
Open tasks: