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.

Sign up for GitHub

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

Jump to bottom

Retry upload overwrite if file locked in UI tests #30513

Merged
merged 1 commit into from
Feb 19, 2018
Merged

Retry upload overwrite if file locked in UI tests #30513

merged 1 commit into from
Feb 19, 2018

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Feb 18, 2018 •
edited
Loading

Description

For file upload tests that overwrite an existing file, retry the upload process if it gets some notification (probably about "file locked"). Report that a notification was received, and the text of the notification, so that it is possible to see in the test output that "something happened" but try again anyway up to the "standard" number of retries.

The retry actions go through the same sequence of UI actions that the user would do anyway, when they see a "file locked" message after trying to do an upload.

Related Issue

#30506

Motivation and Context

Make upload-overwrite UI tests reliable.

How Has This Been Tested?

CI passes

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)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@phil-davis phil-davis self-assigned this Feb 18, 2018
@codecov
Copy link

codecov bot commented Feb 18, 2018 •
edited
Loading

Codecov Report

Merging #30513 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #30513   +/-   ##
=========================================
  Coverage     61.59%   61.59%           
  Complexity    18505    18505           
=========================================
  Files          1090     1090           
  Lines         61104    61104           
=========================================
  Hits          37640    37640           
  Misses        23464    23464

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 88e3fa0...53a6858. Read the comment docs.

@phil-davis
Copy link
Contributor Author

It can retry the UI upload if it gets a file-locked notification, and the upload works the 2nd time - good.

  Scenario: overwrite an existing file in a sub-folder                            # /drone/src/tests/ui/features/upload/upload.feature:70
    When I open the folder "simple-folder"                                        # FilesContext::iOpenTheFolderNamed()
Upload overwriting lorem.txt and got 1 notifications including "simple-folder/lorem.txt" is locked
INFORMATION: retried to upload overwriting file lorem.txt 1 times
    And I upload overwriting the file "lorem.txt" and retry if the file is locked # FilesContext::iUploadOverwritingTheFileRetry()
      │ INFORMATION: retried to upload overwriting file lorem.txt 1 times
    Then the file "lorem.txt" should be listed                                    # FilesContext::theFileFolderShouldBeListed()
    And the content of "lorem.txt" should be the same as the local "lorem.txt"    # FilesContext::theContentOfShouldBeTheSameAsTheLocal()

@phil-davis phil-davis force-pushed the retry-upload-if-file-locked-01 branch 2 times, most recently from 1e98a18 to b5dc81f Compare February 18, 2018 04:43
@phil-davis phil-davis changed the title [WIP] Retry upload if file locked 01 Retry upload overwrite if file locked in UI tests Feb 18, 2018
This was referenced Feb 18, 2018
Closed
Closed
@phil-davis phil-davis added this to the development milestone Feb 18, 2018
@phil-davis phil-davis mentioned this pull request Feb 18, 2018
Closed
individual-it
individual-it previously requested changes Feb 18, 2018
View reviewed changes
tests/ui/features/bootstrap/FeatureContext.php Outdated
public function noNotificationShouldBeDisplayed() {
try {
$notificationText = $this->owncloudPage->getNotificationText();
PHPUnit_Framework_Assert::assertEquals('', $notificationText, "Expecting no notifications but got $notificationText");
Copy link
Member

Choose a reason for hiding this comment

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

pretty long line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will run phpcs ...

tests/ui/features/bootstrap/FeatureContext.php Outdated
try {
$notificationText = $this->owncloudPage->getNotificationText();
PHPUnit_Framework_Assert::assertEquals('', $notificationText, "Expecting no notifications but got $notificationText");
} catch (\SensioLabs\Behat\PageObjectExtension\PageObject\Exception\ElementNotFoundException $e) {
Copy link
Member

Choose a reason for hiding this comment

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

can we put it in a use statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

tests/ui/features/bootstrap/FilesContext.php Outdated
@@ -91,13 +92,15 @@ class FilesContext extends RawMinkContext implements Context {
* @return void
*/
public function __construct(
OwncloudPage $owncloudPage,
Copy link
Member

Choose a reason for hiding this comment

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

why do we need that? Every page extends OwncloudPage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not a Page but a Context class.

Copy link
Member

Choose a reason for hiding this comment

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

class FilesPage extends FilesPageBasic {

core/tests/ui/features/lib/FilesPage.php

Line 35 in eca03d9

class FilesPage extends FilesPageBasic {

abstract class FilesPageBasic extends OwnCloudPage {

core/tests/ui/features/lib/FilesPageBasic.php

Line 35 in eca03d9

abstract class FilesPageBasic extends OwnCloudPage {

so in https://github.com/owncloud/core/pull/30513/files#diff-728c995ac1e138bb2d77c3a588cb018dR481 you should be able to use $notifications = $this->filesPage->getNotifications();

Copy link
Contributor Author

@phil-davis phil-davis Feb 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

now I see :) what you mean

@phil-davis phil-davis force-pushed the retry-upload-if-file-locked-01 branch 2 times, most recently from eca03d9 to 4fe775e Compare February 19, 2018 02:29
@phil-davis phil-davis dismissed individual-it’s stale review February 19, 2018 04:33

changes made - please review again

@phil-davis phil-davis mentioned this pull request Feb 19, 2018
Closed
@phil-davis phil-davis mentioned this pull request Feb 19, 2018
Merged
9 tasks
@phil-davis phil-davis merged commit 0a0d750 into master Feb 19, 2018
@phil-davis phil-davis deleted the retry-upload-if-file-locked-01 branch February 19, 2018 09:59
@phil-davis phil-davis mentioned this pull request Feb 19, 2018
Merged
@phil-davis
Copy link
Contributor Author

Backport stable10 #30528

@lock
Copy link

lock bot commented Aug 1, 2019

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 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@individual-it individual-it individual-it approved these changes

@patrickjahns patrickjahns Awaiting requested review from patrickjahns

Assignees

@phil-davis phil-davis

Labels
3 - To Review dev:acceptance-tests
Projects
None yet
Milestone
10.0.8
Development

Successfully merging this pull request may close these issues.
2 participants
@phil-davis @individual-it