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

[tests-only][full-ci]added then steps for apiMain suite #39802

Merged
merged 1 commit into from
Feb 24, 2022

Conversation

sushmita56
Copy link
Contributor

@sushmita56 sushmita56 commented Feb 17, 2022

Description

This PR adds Then steps to existing API test scenarios to better check the results of When steps.

Suite Covered

  • apiMain

Related Issues

How Has This Been Tested?

  • Locally
  • CI

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@update-docs
Copy link

update-docs bot commented Feb 17, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@CLAassistant
Copy link

CLAassistant commented Feb 17, 2022

CLA assistant check
All committers have signed the CLA.

@sushmita56 sushmita56 changed the title added then steps for apiMain suite [test-only][full-ci]added then steps for apiMain suite Feb 17, 2022
@sushmita56 sushmita56 changed the title [test-only][full-ci]added then steps for apiMain suite [tests-only][full-ci]added then steps for apiMain suite Feb 17, 2022
@sushmita56 sushmita56 self-assigned this Feb 17, 2022
@sushmita56 sushmita56 linked an issue Feb 17, 2022 that may be closed by this pull request
@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline webUIUpload-chrome-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/34682/151/1

@sushmita56 sushmita56 force-pushed the then-steps-in-apiMain branch from ed93b40 to 6b3acd4 Compare February 17, 2022 11:35
@sushmita56
Copy link
Contributor Author

Is there any alternative that we can use instead of repeating When and Then steps again and again?

Copy link
Member

@saw-jan saw-jan left a comment

Choose a reason for hiding this comment

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

LGTM

| dav_version | checksum |
| spaces | SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f MD5:d70b40f177b14b470d1756a3c12b963a ADLER32:8ae90960 |
| dav_version | checksum | response_status |
| spaces | SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f MD5:d70b40f177b14b470d1756a3c12b963a ADLER32:8ae90960 |409 |
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we expect a 409 with "spaces" access?

Copy link
Contributor

Choose a reason for hiding this comment

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

seems like an issue in ocis, the scenario is also not in the expected failures list

Copy link
Contributor

Choose a reason for hiding this comment

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

@kiranparajuli589 maybe we don't even run these features in oCIS??? Please check.

Copy link
Contributor

Choose a reason for hiding this comment

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

this should be running in ocis, but the bug was covered imo, the checksum matches but status is not expected

Copy link
Contributor

Choose a reason for hiding this comment

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

checking it here further owncloud/ocis#3199

Copy link
Contributor

Choose a reason for hiding this comment

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

see the test passing here, https://drone.owncloud.com/owncloud/ocis/9489/23/6
@sushmita56 please use the same status code as it should be in this scenario and later add the failing test to the expected to failures list with linking to the proper issue (may not be registered yet).

@phil-davis
Copy link
Contributor

Is there any alternative that we can use instead of repeating When and Then steps again and again?

We should discuss this. For example:

  Scenario Outline: Copying file with checksum should return the checksum in the download header using new DAV path
    Given using <dav_version> DAV path
    And user "Alice" has uploaded file "filesForUpload/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a"
    When user "Alice" copies file "/myChecksumFile.txt" to "/myChecksumFileCopy.txt" using the WebDAV API
    Then the HTTP status code should be "201"
    When user "Alice" downloads file "/myChecksumFileCopy.txt" using the WebDAV API
    Then the HTTP status code should be "200"
    And the header checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f"
    Examples:
      | dav_version |
      | new         |

The scenario actually wants to test that the checksum is correct when a copy of the file is downloaded. We could change the first When step(s) into Given:

  Scenario Outline: Copying file with checksum should return the checksum in the download header using new DAV path
    Given using <dav_version> DAV path
    And user "Alice" has uploaded file "filesForUpload/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a"
    And user "Alice" has copied file "/myChecksumFile.txt" to "/myChecksumFileCopy.txt"
    When user "Alice" downloads file "/myChecksumFileCopy.txt" using the WebDAV API
    Then the HTTP status code should be "200"
    And the header checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f"
    Examples:
      | dav_version |
      | new         |

The Given step for user "Alice" has copied file... would check the status itself, like we do in other Given steps.

But in this scenario, we are really trying to test that after a file is copied, the checksum is still correct. So the "copy" step is really the action step for the scenario. So we could do:

  Scenario Outline: Copying file with checksum should return the checksum in the download header using new DAV path
    Given using <dav_version> DAV path
    And user "Alice" has uploaded file "filesForUpload/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a"
    When user "Alice" copies file "/myChecksumFile.txt" to "/myChecksumFileCopy.txt" using the WebDAV API
    Then the HTTP status code should be "201"
    And the header checksum when downloading file "/myChecksumFileCopy.txt" using the WebDAV API should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f"
    Examples:
      | dav_version |
      | new         |

We could write a longer Then step he header checksum when downloading file... - we have done that kind of thing in other places. The step would internally download the file, check the HTTP status, and check the checksum.

@sushmita56
Copy link
Contributor Author

sushmita56 commented Feb 21, 2022

What can we do in such scenarios? @phil-davis @kiranparajuli589

  @issue-ocis-reva-56 @notToImplementOnOCIS @newChunking @issue-ocis-1321
  Scenario: Upload new DAV chunked file where checksum matches
    Given using new DAV path
    When user "Alice" creates a new chunking upload with id "chunking-42" using the WebDAV API
    Then the HTTP status code should be "201"
    When user "Alice" uploads new chunk file "2" with "BBBBB" to id "chunking-42" using the WebDAV API
    Then the HTTP status code should be "201"
    When user "Alice" uploads new chunk file "3" with "CCCCC" to id "chunking-42" using the WebDAV API
    Then the HTTP status code should be "201"
    When user "Alice" moves new chunk file with id "chunking-42" to "/myChunkedFile.txt" with checksum "SHA1:5d84d61b03fdacf813640f5242d309721e0629b1" using the WebDAV API
    Then the HTTP status code should be "201"
  Scenario Outline: Sharing and modifying a file should return correct checksum in the propfind using new DAV path
    Given the administrator has set the default folder for received shares to "Shares"
    And auto-accept shares has been disabled
    And using <dav_version> DAV path
    And user "Brian" has been created with default attributes and without skeleton files
    And user "Alice" has uploaded file "filesForUpload/textfile.txt" to "/myChecksumFile.txt" with checksum "MD5:d70b40f177b14b470d1756a3c12b963a"
    When user "Alice" shares file "/myChecksumFile.txt" with user "Brian" using the sharing API
    Then the HTTP status code should be "200"
    When user "Brian" accepts share "/myChecksumFile.txt" offered by user "Alice" using the sharing API
    Then the HTTP status code should be "200"
    When user "Brian" uploads file with checksum "SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399" and content "Some Text" to "/Shares/myChecksumFile.txt" using the WebDAV API
    Then the HTTP status code should be "<response_status>"
    And as user "Alice" the webdav checksum of "/myChecksumFile.txt" via propfind should match "<checksum>"

@kiranparajuli589
Copy link
Contributor

kiranparajuli589 commented Feb 21, 2022

  @issue-ocis-reva-56 @notToImplementOnOCIS @newChunking @issue-ocis-1321
  Scenario: Upload new DAV chunked file where checksum matches
    Given using new DAV path
    When user "Alice" creates a new chunking upload with id "chunking-42" using the WebDAV API
    Then the HTTP status code should be "201"
    When user "Alice" uploads new chunk file "2" with "BBBBB" to id "chunking-42" using the WebDAV API
    Then the HTTP status code should be "201"
    When user "Alice" uploads new chunk file "3" with "CCCCC" to id "chunking-42" using the WebDAV API
    Then the HTTP status code should be "201"
    When user "Alice" moves new chunk file with id "chunking-42" to "/myChunkedFile.txt" with checksum "SHA1:5d84d61b03fdacf813640f5242d309721e0629b1" using the WebDAV API
    Then the HTTP status code should be "201"

What can we do in such a scenario? @phil-davis @kiranparajuli589

  • introducing an example table for such repeated steps can reduce the number involving when steps but again there will be 3 when steps for this single scenario.
  • first when can be switched to given IMO because it involves creating
  • if we don't feel like moving to Given and still have no. of when then what to do 🤔 @phil-davis if we have multiple whens then there can be multiple Thens beneath every when step.

@phil-davis
Copy link
Contributor

When user "Alice" does a chunking upload with id "chunking-42" to file "/myChunkedFile.txt" with checksum "SHA1:5d84d61b03fdacf813640f5242d309721e0629b1" and the following chunks using the WebDAV API
  | part | data  |
  | 2     | BBBBB |
  | 3     | CCCCC |
Then the HTTP status of all the chunking upload requests should be "201"

The When step can remember the status of each request - the request that creates id "chunking-42", each request that uploads a chunk, and the final request that moves the "chunking-42" parts to "myChunkedFile.txt"

The Then step can check each request. Probably it can fail immediately that it detects a wrong status - if an early request goes wrong, then the status of the later requests is not really interesting, they are also going to fail.

A lot of the detailed chunking-upload tests could be refactored like that.

@sushmita56 sushmita56 force-pushed the then-steps-in-apiMain branch from 6b3acd4 to 6fa74d2 Compare February 23, 2022 07:02
@grgprarup grgprarup self-requested a review February 23, 2022 08:04
@sushmita56 sushmita56 force-pushed the then-steps-in-apiMain branch from 6fa74d2 to ea2a219 Compare February 23, 2022 09:54
@sushmita56 sushmita56 force-pushed the then-steps-in-apiMain branch from ea2a219 to 90524bd Compare February 23, 2022 11:09
@sushmita56 sushmita56 force-pushed the then-steps-in-apiMain branch 2 times, most recently from 8957582 to 7cdfc5b Compare February 23, 2022 11:18
added then steps for apiMain suite

improved when then steps

addressed reviews
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Contributor

@kiranparajuli589 kiranparajuli589 left a comment

Choose a reason for hiding this comment

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

lgtm 👍

And user "Alice" uploads new chunk file "3" with "CCCCC" to id "chunking-42" using the WebDAV API
And user "Alice" moves new chunk file with id "chunking-42" asynchronously to "/myChunkedFile.txt" with checksum "SHA1:f005ba11" using the WebDAV API
And user "Alice" moves new chunk file with id "chunking-42" asynchronously to "/myChunkedFile.txt" with checksum "SHA1:5d84d61b03fdacf813640f5242d309721e0629b1" using the WebDAV API
Then the HTTP status code should be "202"
Then the HTTP status code of responses on each endpoint should be "201, 201, 202, 202" respectively
Copy link
Contributor

Choose a reason for hiding this comment

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

That is strange that the first attempt to move chunkinf-42 to myChunkedFile.txt gives a 202 response - the response indicates that it worked, but actually it should not work. We should make an issue and bug-demo scenario about that - I will raise an issue.

@phil-davis phil-davis merged commit 8203cf2 into master Feb 24, 2022
@delete-merged-branch delete-merged-branch bot deleted the then-steps-in-apiMain branch February 24, 2022 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants