[QA] Check and remove the use of `setResponse()` and `$this->response` in the Given/Then steps #7082

saw-jan · 2023-08-21T11:48:19Z

We have used setResponse() and $this->response in the Given/Then steps and some helper functions (maybe to reuse existing available methods). But storing responses from Given/Then steps and helper functions is not a good idea because it can lead to a false positive assertion in the Then steps.

For example,

ocis/tests/acceptance/features/coreApiVersions/fileVersions.feature

Lines 18 to 21 in ff304df

    
           Scenario: upload file and no version is available using various chunking methods (except new chunking) 
        
             When user "Alice" uploads file "filesForUpload/davtest.txt" to filenames based on "/davtest.txt" with all mechanisms except new chunking using the WebDAV API 
        
             Then the HTTP status code should be "200" 
        
             And the version folder of file "/davtest.txt-olddav-regular" for user "Alice" should contain "0" elements

Here, the When step is upload resource and Then step checks that the When step's response code is 200 which is wrong here but the test passes. This is because of the saved response from Given step.

Open tasks:

check the use of setResponse() and $this->response in
- Given steps
- Then steps (Then steps can use $this->response but must prevent saving to it)
- Helper functions
Prevent use of step-def functions in other steps-defs wherever possible (strictly prevent the reuse between cross steps).
Refactor theHTTPStatusCodeShouldBe() so that it can accept response object to get statuscode from [tests-only][full-CI] make httpresponse check function able to accept response object as parameter #7169
```
theHTTPStatusCodeShouldBe($response)
```

The text was updated successfully, but these errors were encountered:

amrita-shrestha · 2023-08-22T06:43:36Z

maybe this old issue help owncloud/core#39512 and owncloud/QA#517

In the past, we emptied HTTP status code in the Given step to prevent such incidents. If that didn't prevent false positive then we have to discuss and brainstorm different ways. cc @saw-jan @SwikritiT @SagarGi @grgprarup @PrajwolAmatya

	/**
	 * @When user :user uploads file :source to :destination using the WebDAV API
	 *
	 * @param string $user
	 * @param string $source
	 * @param string $destination
	 *
	 * @return void
	 */
	public function userUploadsAFileTo(string $user, string $source, string $destination):void {
		$user = $this->getActualUsername($user);
		$file = \fopen($this->acceptanceTestsDirLocation() . $source, 'r');
		$this->pauseUploadDelete();
		$this->response = $this->makeDavRequest(
			$user,
			"PUT",
			$destination,
			[],
			$file
		);
		$this->lastUploadDeleteTime = \time();
		$this->setResponseXml(
			HttpRequestHelper::parseResponseAsXml($this->response)
		);
		$this->pushToLastHttpStatusCodesArray(
			(string) $this->getResponse()->getStatusCode()
		);
	}

	/**
	 * @Given user :user has uploaded file :source to :destination
	 *
	 * @param string $user
	 * @param string $source
	 * @param string $destination
	 *
	 * @return void
	 */
	public function userHasUploadedAFileTo(string $user, string $source, string $destination):void {
		$this->userUploadsAFileTo($user, $source, $destination);
		$this->theHTTPStatusCodeShouldBe(
			["201", "204"],
			"HTTP status code was not 201 or 204 while trying to upload file '$source' to '$destination' for user '$user'"
		);
		$this->emptyLastHTTPStatusCodesArray();
	}

saw-jan · 2023-08-22T06:57:03Z

Never save responses in Given steps. Let the response scope be only within the Given steps. That way we don't have to worry about clearing the response/statuscode stores.

amrita-shrestha · 2023-08-22T07:01:35Z

so there is two tasks here

remove the response saved by setResponse()
clean previously saved HTTP step by pushToLastHttpStatusCodesArray()

Am I correct @saw-jan

saw-jan · 2023-08-22T07:04:41Z

If those are done in Given/Then steps or helper functions then yes. But if you mean everywhere in the test code then no.
setResponse() and pushToLastHttpStatusCodesArray() should not be used in Given/Then and helper functions

amrita-shrestha · 2023-08-22T07:16:45Z

recently something catches my attention we call When step function in Given step. So Given and When are interlinked so how could we remove setResponse(). cc @saw-jan

saw-jan · 2023-08-22T07:20:47Z

Yeah, that should be part of this ticket.
we should not and never use step-def functions in other steps-defs wherever possible (strictly not between cross steps).
What we can do in that case:

create a helper function
use the helper function in the step definition function

phil-davis · 2023-08-22T08:18:06Z

use the helper function in the step definition function

yes, I agree. We did this sort of pattern in quite a lot of places already. Forexample, I see in FavoritesContext (happens to be in core, but similar examples will be in ocis test code:

	public function userFavoritesElement(string $user, string $path):void {
		$response = $this->changeFavStateOfAnElement(
			$user,
			$path,
			1
		);
		$this->featureContext->setResponse($response);
	}

	/**
	 * @When user :user favorites element :path using the WebDAV API
	 */
	public function userFavoritesElementUsingWebDavApi(string $user, string $path):void {
		$this->userFavoritesElement($user, $path);
	}

	/**
	 * @Given user :user has favorited element :path
	 */
	public function userHasFavoritedElementUsingWebDavApi(string $user, string $path):void {
		$this->userFavoritesElement($user, $path);
		$this->featureContext->theHTTPStatusCodeShouldBeSuccess();
	}

The setResponse call can be moved into the When step code, so that only the When step remembers the response.

saw-jan · 2023-08-25T11:29:43Z

If you go on refactoring Given steps and helper functions then you will know which test suites are affected. So, IMO, this ticket cannot be divided by test suites. But I am just letting you know. You can continue your way.

KarunAtreya · 2023-08-29T04:20:41Z

If you go on refactoring Given steps and helper functions then you will know which test suites are affected. So, IMO, this ticket cannot be divided by test suites. But I am just letting you know. You can continue your way.

should i go context wise then?

saw-jan · 2023-08-29T04:24:35Z

should i go context wise then?

maybe yes.

KarunAtreya · 2023-09-04T04:00:05Z

TODO:

Master

stable-4.0

SwikritiT · 2023-09-06T07:53:45Z

@KarunAtreya I've updated the above checklist to include the backport checklist as well. Can you please backport your related PRs and link them there

nirajacharya2 · 2023-12-15T04:30:24Z

Salipa-Gurung · 2024-01-15T10:38:09Z

Prevent use of step-def functions in other steps-defs wherever possible (strictly prevent the reuse between cross steps).
We will continue to work on this in next issue #8199

saw-jan added the QA:team label Aug 21, 2023

amrita-shrestha assigned KarunAtreya Aug 22, 2023

KarunAtreya mentioned this issue Aug 25, 2023

[tests-only][full-ci] removing the setresponse in given/then step in FileVersionsContext #7111

Merged

9 tasks

This was referenced Aug 31, 2023

[tests-only][full-CI] make httpresponse check function able to accept response object as parameter #7169

Merged

[tests-only][full-ci] removing the setresponse in given/then step in AppConfigurationContext and AuthContext #7186

Merged

KarunAtreya mentioned this issue Sep 4, 2023

[tests-only][full-ci] removing the setresponse in given/then step in CheckSumContext and FavoritesContext #7202

Merged

9 tasks

This was referenced Sep 7, 2023

[tests-only][full-ci] backport removing the setresponse in given/then step in AppConfigurationContext and AuthContext #7244

Merged

[tests-only][full-ci] removing the setresponse in given/then step in TrashbinContext and TagsContext #7261

Merged

grgprarup assigned nirajacharya2 Oct 2, 2023

nabim777 assigned nabim777 and unassigned nabim777 Oct 3, 2023

KarunAtreya mentioned this issue Oct 5, 2023

[tests-only] [full-ci] removing the setresponse in given/then step in webdav context #7403

Merged

9 tasks

nirajacharya2 mentioned this issue Oct 5, 2023

[tests-only][full-ci] removing the setResponse() in given/then step in GraphContext #7399

Merged

9 tasks

nirajacharya2 mentioned this issue Nov 2, 2023

[tests-only][full-ci] removing the setResponse() in given/then step in GraphContext #7646

Merged

9 tasks

grgprarup unassigned KarunAtreya Nov 6, 2023

This was referenced Nov 27, 2023

[tests-only][full-ci] removing the setResponse() and $this->response in given/then step in Provisioning file #7811

Merged

[tests-only][full-ci] removing the setResponse() and $this->response in given/then step in FeatureContext file #7822

Merged

This was referenced Dec 6, 2023

[tests-only][full-ci] removing the setResponse() and $this->response in given/then step in Sharing file #7889

Merged

[tests-only][full-ci] removing the setResponse() and $this->response in given/then step in Provisioning file #7867

Merged

Salipa-Gurung mentioned this issue Jan 15, 2024

[QA] Prevent use of step-def functions in other steps-defs in tests #8199

Closed

27 tasks

Salipa-Gurung closed this as completed Jan 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[QA] Check and remove the use of `setResponse()` and `$this->response` in the Given/Then steps #7082

[QA] Check and remove the use of `setResponse()` and `$this->response` in the Given/Then steps #7082

saw-jan commented Aug 21, 2023 •

edited by nirajacharya2

Loading

amrita-shrestha commented Aug 22, 2023 •

edited

Loading

saw-jan commented Aug 22, 2023 •

edited

Loading

amrita-shrestha commented Aug 22, 2023

saw-jan commented Aug 22, 2023 •

edited

Loading

amrita-shrestha commented Aug 22, 2023

saw-jan commented Aug 22, 2023

phil-davis commented Aug 22, 2023

saw-jan commented Aug 25, 2023 •

edited

Loading

KarunAtreya commented Aug 29, 2023

saw-jan commented Aug 29, 2023

KarunAtreya commented Sep 4, 2023 •

edited by nirajacharya2

Loading

SwikritiT commented Sep 6, 2023 •

edited

Loading

nirajacharya2 commented Dec 15, 2023 •

edited by grgprarup

Loading

Salipa-Gurung commented Jan 15, 2024

[QA] Check and remove the use of setResponse() and $this->response in the Given/Then steps #7082

[QA] Check and remove the use of setResponse() and $this->response in the Given/Then steps #7082

Comments

saw-jan commented Aug 21, 2023 • edited by nirajacharya2 Loading

amrita-shrestha commented Aug 22, 2023 • edited Loading

saw-jan commented Aug 22, 2023 • edited Loading

amrita-shrestha commented Aug 22, 2023

saw-jan commented Aug 22, 2023 • edited Loading

amrita-shrestha commented Aug 22, 2023

saw-jan commented Aug 22, 2023

phil-davis commented Aug 22, 2023

saw-jan commented Aug 25, 2023 • edited Loading

KarunAtreya commented Aug 29, 2023

saw-jan commented Aug 29, 2023

KarunAtreya commented Sep 4, 2023 • edited by nirajacharya2 Loading

Master

stable-4.0

SwikritiT commented Sep 6, 2023 • edited Loading

nirajacharya2 commented Dec 15, 2023 • edited by grgprarup Loading

Master

stable-5.0

Salipa-Gurung commented Jan 15, 2024

[QA] Check and remove the use of `setResponse()` and `$this->response` in the Given/Then steps #7082

[QA] Check and remove the use of `setResponse()` and `$this->response` in the Given/Then steps #7082

saw-jan commented Aug 21, 2023 •

edited by nirajacharya2

Loading

amrita-shrestha commented Aug 22, 2023 •

edited

Loading

saw-jan commented Aug 22, 2023 •

edited

Loading

saw-jan commented Aug 22, 2023 •

edited

Loading

saw-jan commented Aug 25, 2023 •

edited

Loading

KarunAtreya commented Sep 4, 2023 •

edited by nirajacharya2

Loading

SwikritiT commented Sep 6, 2023 •

edited

Loading

nirajacharya2 commented Dec 15, 2023 •

edited by grgprarup

Loading