Skip to content

MFTF-33782: Added empty query and fragment testing to the UrlFormatterTest #867

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

Merged
merged 3 commits into from
Aug 18, 2021
Merged

MFTF-33782: Added empty query and fragment testing to the UrlFormatterTest #867

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
41bd267
MFTF-33782: Added empty query and fragment testing to the UrlFormatte…
bohdan-harniuk Aug 16, 2021
2a40fdf
MFTF-33782: Code refactoring
bohdan-harniuk Aug 16, 2021
860a39e
MFTF-33782: Made NULL value equal to the default argument value
bohdan-harniuk Aug 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 25 additions & 20 deletions dev/tests/unit/Magento/FunctionalTestFramework/Util/Path/FilePathFormatterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,25 +3,28 @@
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace tests\unit\Magento\FunctionalTestFramework\Util\Path;

use Magento\FunctionalTestingFramework\Exceptions\TestFrameworkException;
use tests\unit\Util\MagentoTestCase;
use Magento\FunctionalTestingFramework\Util\Path\FilePathFormatter;
use tests\unit\Util\MagentoTestCase;

class FilePathFormatterTest extends MagentoTestCase
{
/**
* Test file format
* Test file format.
*
* @dataProvider formatDataProvider
* @param string $path
* @param boolean $withTrailingSeparator
* @param mixed string|boolean $expectedPath
* @param bool $withTrailingSeparator
* @param string|null $expectedPath
*
* @return void
* @throws TestFrameworkException
* @dataProvider formatDataProvider
*/
public function testFormat($path, $withTrailingSeparator, $expectedPath)
public function testFormat(string $path, bool $withTrailingSeparator, ?string $expectedPath): void
{
if (null !== $expectedPath) {
$this->assertEquals($expectedPath, FilePathFormatter::format($path, $withTrailingSeparator));
Expand All @@ -33,52 +36,54 @@ public function testFormat($path, $withTrailingSeparator, $expectedPath)
}

/**
* Test file format with exception
* Test file format with exception.
*
* @dataProvider formatExceptionDataProvider
* @param string $path
* @param boolean $withTrailingSeparator
* @param bool $withTrailingSeparator
*
* @return void
* @throws TestFrameworkException
* @dataProvider formatExceptionDataProvider
*/
public function testFormatWithException($path, $withTrailingSeparator)
public function testFormatWithException(string $path, bool $withTrailingSeparator): void
{
$this->expectException(TestFrameworkException::class);
$this->expectExceptionMessage("Invalid or non-existing file: $path\n");
FilePathFormatter::format($path, $withTrailingSeparator);
}

/**
* Data input
* Data input.
*
* @return array
*/
public function formatDataProvider()
public function formatDataProvider(): array
{
$path1 = rtrim(TESTS_BP, '/');
$path2 = $path1 . DIRECTORY_SEPARATOR;

return [
[$path1, null, $path1],
[$path1, false, $path1],
Copy link
Contributor

@jilu1 jilu1 Aug 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@bohdan-harniuk
I see you have changed null to false in many places for data input. Isn't it the opposite of the original test. Am I missing something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, @jilu1!

I've done it after the small investigation. I believe that bool variables should have only two states: TRUE or FALSE.

There are 55 places, in the MFTF source code, where FilePathFormatter::format method is called.
Also, there are 17 places in the MFTF source code where UrlFormatter::format method is called.

I've checked all those places, if in any of them there is null value passed into those methods. There is no such place.
So, I decided to limit the value of this variable: TRUE or FALSE.
The NULL values was passed only during testing for both cases.

Do we have some other intentions to allow pass here NULL value (?bool instead of bool)?

Thanks, Bohdan

Copy link
Contributor

Choose a reason for hiding this comment

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

@bohdan-harniuk
Maybe we can use ?bool here. The intention is when NULL is used, the relevant parameter is omitted and thus to test using the default value in FilePathFormatter::format or UrlFormatter::format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jilu1
It makes sense! I'll rollback NULL values.
Thank you!

[$path1, false, $path1],
[$path1, true, $path2],
[$path2, null, $path1],
[$path2, false, $path1],
[$path2, false, $path1],
[$path2, true, $path2],
[__DIR__. DIRECTORY_SEPARATOR . basename(__FILE__), null, __FILE__],
['', null, null] // Empty string is valid
[__DIR__. DIRECTORY_SEPARATOR . basename(__FILE__), false, __FILE__],
['', false, null] // Empty string is valid
];
}

/**
* Invalid data input
* Invalid data input.
*
* @return array
*/
public function formatExceptionDataProvider()
public function formatExceptionDataProvider(): array
{
return [
['abc', null],
['X://some\dir/@', null],
['abc', false],
['X://some\dir/@', false]
];
}
}
50 changes: 28 additions & 22 deletions dev/tests/unit/Magento/FunctionalTestFramework/Util/Path/UrlFormatterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,51 +3,53 @@
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace tests\unit\Magento\FunctionalTestFramework\Util\Path;

use tests\unit\Util\MagentoTestCase;
use Magento\FunctionalTestingFramework\Util\Path\UrlFormatter;
use Magento\FunctionalTestingFramework\Exceptions\TestFrameworkException;
use Magento\FunctionalTestingFramework\Util\Path\UrlFormatter;
use tests\unit\Util\MagentoTestCase;

class UrlFormatterTest extends MagentoTestCase
{
/**
* Test url format
* Test url format.
*
* @dataProvider formatDataProvider
* @param string $path
* @param boolean $withTrailingSeparator
* @param mixed string|boolean $expectedPath
* @param bool $withTrailingSeparator
* @param string $expectedPath
*
* @return void
* @throws TestFrameworkException
* @dataProvider formatDataProvider
*/
public function testFormat($path, $withTrailingSeparator, $expectedPath)
public function testFormat(string $path, bool $withTrailingSeparator, string $expectedPath): void
{
$this->assertEquals($expectedPath, UrlFormatter::format($path, $withTrailingSeparator));
}

/**
* Test url format with exception
* Test url format with exception.
*
* @dataProvider formatExceptionDataProvider
* @param string $path
* @param boolean $withTrailingSeparator
* @param bool $withTrailingSeparator
*
* @return void
* @throws TestFrameworkException
* @dataProvider formatExceptionDataProvider
*/
public function testFormatWithException($path, $withTrailingSeparator)
public function testFormatWithException(string $path, bool $withTrailingSeparator): void
{
$this->expectException(TestFrameworkException::class);
$this->expectExceptionMessage("Invalid url: $path\n");
UrlFormatter::format($path, $withTrailingSeparator);
}

/**
* Data input
* Data input.
*
* @return array
*/
public function formatDataProvider()
public function formatDataProvider(): array
{
$url1 = 'http://magento.local/index.php';
$url2 = $url1 . '/';
Expand All @@ -58,34 +60,38 @@ public function formatDataProvider()
$url7 = 'http://127.0.0.1:8200';
$url8 = 'wwøw.goåoøgle.coøm';
$url9 = 'http://www.google.com';

return [
[$url1, null, $url1],
[$url1, false, $url1],
[$url1, false, $url1],
[$url1, true, $url2],
[$url2, null, $url1],
[$url2, false, $url1],
[$url2, false, $url1],
[$url2, true, $url2],
[$url3, null, $url3],
[$url3, false, $url3],
[$url3, false, $url3],
[$url3, true, $url4],
[$url4, null, $url3],
[$url4, false, $url3],
[$url4, false, $url3],
[$url4, true, $url4],
[$url5, true, $url6],
[$url7, false, $url7],
[$url8, false, $url9],
['https://magento.local/path?', false, 'https://magento.local/path?'],
['https://magento.local/path#', false, 'https://magento.local/path#'],
['https://magento.local/path?#', false, 'https://magento.local/path?#']
];
}

/**
* Invalid data input
* Invalid data input.
*
* @return array
*/
public function formatExceptionDataProvider()
public function formatExceptionDataProvider(): array
{
return [
['', null],
['', false]
];
}
}
6 changes: 4 additions & 2 deletions src/Magento/FunctionalTestingFramework/Util/Path/FilePathFormatter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\FunctionalTestingFramework\Util\Path;

Expand All @@ -11,14 +12,15 @@
class FilePathFormatter implements FormatterInterface
{
/**
* Return formatted full file path from input string, or false on error
* Return formatted full file path from input string, or false on error.
*
* @param string $path
* @param boolean $withTrailingSeparator
*
* @return string
* @throws TestFrameworkException
*/
public static function format($path, $withTrailingSeparator = true)
public static function format(string $path, bool $withTrailingSeparator = true): string
{
$validPath = realpath($path);

Expand Down
6 changes: 4 additions & 2 deletions src/Magento/FunctionalTestingFramework/Util/Path/FormatterInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\FunctionalTestingFramework\Util\Path;

Expand All @@ -11,12 +12,13 @@
interface FormatterInterface
{
/**
* Return formatted path (file path, url, etc) from input string, or false on error
* Return formatted path (file path, url, etc) from input string, or false on error.
*
* @param string $input
* @param boolean $withTrailingSeparator
*
* @return string
* @throws TestFrameworkException
*/
public static function format($input, $withTrailingSeparator = true);
public static function format(string $input, bool $withTrailingSeparator = true): string;
}
61 changes: 37 additions & 24 deletions src/Magento/FunctionalTestingFramework/Util/Path/UrlFormatter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\FunctionalTestingFramework\Util\Path;

Expand All @@ -11,14 +12,15 @@
class UrlFormatter implements FormatterInterface
{
/**
* Return formatted url path from input string
* Return formatted url path from input string.
*
* @param string $url
* @param boolean $withTrailingSeparator
*
* @return string
* @throws TestFrameworkException
*/
public static function format($url, $withTrailingSeparator = true)
public static function format(string $url, bool $withTrailingSeparator = true): string
{
$sanitizedUrl = rtrim($url, '/');

Expand Down Expand Up @@ -47,12 +49,13 @@ public static function format($url, $withTrailingSeparator = true)
}

/**
* Try to build missing url scheme and host
* Try to build missing url scheme and host.
*
* @param string $url
*
* @return string
*/
private static function buildUrl($url)
private static function buildUrl(string $url): string
{
$urlParts = parse_url($url);

Expand All @@ -76,32 +79,42 @@ private static function buildUrl($url)
/**
* Returns url from $parts given, used with parse_url output for convenience.
* This only exists because of deprecation of http_build_url, which does the exact same thing as the code below.
*
* @param array $parts
*
* @return string
*/
private static function merge(array $parts)
private static function merge(array $parts): string
{
$get = function ($key) use ($parts) {
return isset($parts[$key]) ? $parts[$key] : null;
return $parts[$key] ?? '';
};

$pass = $get('pass');
$user = $get('user');
$userinfo = $pass !== null ? "$user:$pass" : $user;
$port = $get('port');
$scheme = $get('scheme');
$query = $get('query');
$fragment = $get('fragment');
$authority =
($userinfo !== null ? "$userinfo@" : '') .
$get('host') .
($port ? ":$port" : '');

return
(strlen($scheme) ? "$scheme:" : '') .
(strlen($authority) ? "//$authority" : '') .
$get('path') .
(strlen($query) ? "?$query" : '') .
(strlen($fragment) ? "#$fragment" : '');
$pass = $get('pass');
$user = $get('user');
$userinfo = $pass !== '' ? "$user:$pass" : $user;
$port = $get('port');
$scheme = $get('scheme');
$query = $get('query');
$fragment = $get('fragment');
$authority = ($userinfo !== '' ? "$userinfo@" : '') . $get('host') . ($port ? ":$port" : '');

return str_replace(
[
'%scheme',
'%authority',
'%path',
'%query',
'%fragment'
],
[
strlen($scheme) ? "$scheme:" : '',
strlen($authority) ? "//$authority" : '',
$get('path'),
strlen($query) ? "?$query" : '',
strlen($fragment) ? "#$fragment" : ''
],
'%scheme%authority%path%query%fragment'
);
}
}