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] add tests for checking the header location for tus upload #10807

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

nabim777
Copy link
Member

@nabim777 nabim777 commented Dec 24, 2024

Description

Since current test uses TUS library for tus operations and is unable to get and check the response.
Due to this, there was no check for the location header on TUS upload.
So, in this PR, following things are done:

  • TUS wrapper class is made and able to check the header response.
  • added step on tus scenario for status code check

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment:
  • CI
  • locally
  • ...

Screenshots (if appropriate):

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 only (no source changes)

Checklist:

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

@nabim777 nabim777 self-assigned this Dec 24, 2024
@nabim777 nabim777 force-pushed the tests/check-header-location-for-tus-upload branch from b79bb57 to e9d5edd Compare December 24, 2024 11:07
@nabim777 nabim777 force-pushed the tests/check-header-location-for-tus-upload branch 2 times, most recently from e61de04 to c78d8a1 Compare December 27, 2024 04:47
@nabim777 nabim777 marked this pull request as ready for review December 27, 2024 04:48
@nabim777 nabim777 marked this pull request as draft December 27, 2024 04:48
@nabim777 nabim777 force-pushed the tests/check-header-location-for-tus-upload branch from c78d8a1 to 0ec975d Compare January 2, 2025 07:00
@nabim777 nabim777 marked this pull request as ready for review January 2, 2025 08:29
tests/acceptance/TestHelpers/TusClientWrapper.php Outdated Show resolved Hide resolved
tests/acceptance/TestHelpers/TusClientWrapper.php Outdated Show resolved Hide resolved
tests/acceptance/TestHelpers/TusClientWrapper.php Outdated Show resolved Hide resolved
tests/acceptance/TestHelpers/TusClientWrapper.php Outdated Show resolved Hide resolved
@nabim777 nabim777 force-pushed the tests/check-header-location-for-tus-upload branch from ee5defd to 91ea534 Compare January 3, 2025 04:22
@nabim777 nabim777 requested a review from nirajacharya2 January 3, 2025 04:23
@nabim777 nabim777 force-pushed the tests/check-header-location-for-tus-upload branch from 91ea534 to 0842b80 Compare January 3, 2025 05:17
@nabim777 nabim777 requested a review from phil-davis January 3, 2025 06:21
@nabim777 nabim777 force-pushed the tests/check-header-location-for-tus-upload branch from 0842b80 to d592bae Compare January 3, 2025 08:19
Signed-off-by: nabim777 <[email protected]>
@nabim777 nabim777 force-pushed the tests/check-header-location-for-tus-upload branch 2 times, most recently from b56281f to b8211ed Compare January 6, 2025 10:50
@nabim777 nabim777 force-pushed the tests/check-header-location-for-tus-upload branch 2 times, most recently from 918dca1 to d5b8065 Compare January 7, 2025 03:57
@nabim777 nabim777 force-pushed the tests/check-header-location-for-tus-upload branch from d5b8065 to abcb354 Compare January 7, 2025 05:29
@nabim777 nabim777 requested a review from PrajwolAmatya January 7, 2025 05:30
Copy link
Member

@SagarGi SagarGi left a comment

Choose a reason for hiding this comment

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

There are lots of line breaks. May be fix them also.

tests/acceptance/TestHelpers/TusClient.php Outdated Show resolved Hide resolved
tests/acceptance/TestHelpers/TusClient.php Outdated Show resolved Hide resolved
tests/acceptance/TestHelpers/TusClient.php Outdated Show resolved Hide resolved
tests/acceptance/TestHelpers/TusClient.php Outdated Show resolved Hide resolved
tests/acceptance/TestHelpers/TusClient.php Outdated Show resolved Hide resolved
tests/acceptance/TestHelpers/TusClient.php Outdated Show resolved Hide resolved
@nabim777 nabim777 requested a review from SagarGi January 7, 2025 09:12
@nabim777 nabim777 force-pushed the tests/check-header-location-for-tus-upload branch 2 times, most recently from 5e17628 to f858b02 Compare January 7, 2025 09:37
@nabim777 nabim777 force-pushed the tests/check-header-location-for-tus-upload branch from f858b02 to 373c077 Compare January 7, 2025 10:30
@nabim777 nabim777 requested a review from S-Panta January 7, 2025 10:31
@nabim777 nabim777 force-pushed the tests/check-header-location-for-tus-upload branch from 373c077 to db2f7b6 Compare January 7, 2025 10:53
Signed-off-by: nabim777 <[email protected]>
@nabim777 nabim777 force-pushed the tests/check-header-location-for-tus-upload branch from db2f7b6 to c860b81 Compare January 7, 2025 10:56
@@ -309,7 +320,7 @@ public function uploadFileUsingTus(
$headers = \array_merge($headers, $checksumHeader);
}

$client = new Client(
$tusClient = new TusClient(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$tusClient = new TusClient(
$client = new TusClient(

* @throws GuzzleException
* @throws ReflectionException
*/
public function publicUploadFileUsingTus(
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 use the above existing method (update if necessary)?

Signed-off-by: nabim777 <[email protected]>
@nabim777 nabim777 requested a review from saw-jan January 14, 2025 09:45
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