Skip to content
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

Merged
merged 1 commit into from
Sep 12, 2018
Merged

[stable10] tests async operations #32594

merged 1 commit into from
Sep 12, 2018

Conversation

individual-it
Copy link
Member

@individual-it individual-it commented Sep 5, 2018

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@individual-it individual-it added this to the development milestone Sep 5, 2018
@individual-it individual-it self-assigned this Sep 5, 2018
@individual-it individual-it force-pushed the stable10-testAsync branch 3 times, most recently from 33fe2dc to ad21a2d Compare September 6, 2018 17:28
@individual-it individual-it changed the title [WIP] [stable10] tests async operations [stable10] tests async operations Sep 6, 2018
$stream = false;
if ($type === "asynchronously") {
$headers['OC-LazyOps'] = 'true';
if ($this->httpRequestTimeout > 0) {
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

@PVince81 PVince81 left a 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 ?

@individual-it
Copy link
Member Author

If someone sets the setting needlessly the server would wait, but if the correct test steps are used it should not be a problem.
The client also can kill the connection by setting the timeout. e.g. with the Given the HTTP-Request-timeout is set to 5 seconds step

Copy link
Contributor

@PVince81 PVince81 left a 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(
Copy link
Contributor

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

Copy link
Member Author

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(
Copy link
Contributor

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)

@PVince81
Copy link
Contributor

a wild conflict appears

@codecov
Copy link

codecov bot commented Sep 11, 2018

Codecov Report

❗ No coverage uploaded for pull request base (stable10@9ec0907). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             stable10   #32594   +/-   ##
===========================================
  Coverage            ?   62.92%           
  Complexity          ?    18853           
===========================================
  Files               ?     1235           
  Lines               ?    73911           
  Branches            ?     1282           
===========================================
  Hits                ?    46511           
  Misses              ?    27020           
  Partials            ?      380
Flag Coverage Δ Complexity Δ
#javascript 53.16% <ø> (?) 0 <ø> (?)
#phpunit 63.98% <ø> (?) 18853 <ø> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ec0907...f4beb74. Read the comment docs.

@individual-it
Copy link
Member Author

@PVince81 just killed that wild beast

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@phil-davis phil-davis left a 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
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

if they were set

@individual-it
Copy link
Member Author

@phil-davis addressed your comments

@phil-davis
Copy link
Contributor

👍
@PVince81 please merge when CI is happy.

@phil-davis
Copy link
Contributor

Drone network errors - restarted.

@PVince81
Copy link
Contributor

restarted again

@phil-davis
Copy link
Contributor

@PVince81 now it is ready for merge.

@PVince81 PVince81 merged commit ba827ea into stable10 Sep 12, 2018
@PVince81 PVince81 deleted the stable10-testAsync branch September 12, 2018 05:28
@PVince81 PVince81 modified the milestones: development, QA Jan 11, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 11, 2020
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.

3 participants