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

[bohdan-harniuk]

Description

I've investigated the impact of the new changes in parse_url() function for PHP 8 on the current MFTF codebase.
Reported change (from the official documentation):

parse_url() will now distinguish absent and empty queries and fragments:

http://example.com/foo → query = null, fragment = null
http://example.com/foo? → query = "", fragment = null
http://example.com/foo# → query = null, fragment = ""
http://example.com/foo?# → query = "", fragment = ""

Previously all cases resulted in query and fragment being null.

In the MFTF codebase this function used only in the \Magento\FunctionalTestingFramework\Util\Path\UrlFormatter class.

I've added two data sets to the \tests\unit\Magento\FunctionalTestFramework\Util\Path\UrlFormatterTest::formatDataProvider to cover cases when parsed URL has an empty query, an empty fragment or both of them.

The result of testing for the PHP7.3:

The result of testing for the PHP8.0:

During this investigation I've found that the \Magento\FunctionalTestingFramework\Util\Path\UrlFormatter class is not strictly typed and has errors regarding strict typing:

Investigation summary

I propose to extend the UrlFormatterTest with the new data sets. If the core team of the MFTF framework will consider those new datasets useful. If not, anyway, this also could be closed and taken into account only the result of this investigation regarding the related issue.

Also, I propose to make the UrlFormatter class strictly typed and fix strict type issues in the scope of this PR.

Related Issues

1. [MFTF] Investigate and fix code related to changes in parse_url()  magento2#33782: [MFTF] Investigate and fix code related to changes in parse_url() #33782

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/verification tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)
• Changes to Framework doesn't have backward incompatible changes for tests or have related Pull Request with fixes to tests

[jilu1]

@bohdan-harniuk
Thank you for your pull request!
Please go ahead with the change to make UrlFormatter class strictly typed and fix strict type issues.

[bohdan-harniuk]

@jilu1, could you please proceed with the code review?

I had to change few places to make everything in the right way.

Thanks, Bohdan

[jilu1]

@magento import pull request to https://github.com/magento-commerce/magento2-functional-testing-framework

[magento-engcom-team]

@jilu1 the pull request successfully imported.

[jilu1]

@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?

[bohdan-harniuk]

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

[jilu1]

@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

[bohdan-harniuk]

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

[jilu1]

Please see my comment and clarify. Thanks!

[bohdan-harniuk]

Please see my comment and clarify. Thanks!

Hello, @jilu1!

I've made required modifications. Now, NULL value is equal to the default argument value. Please, pay attention, I had to change expected values for the datasets with the NULL value for trailing separator boolean argument. It is done because previously we didn't check default value but the opposite -> NULL was interpreted as FALSE.

Thanks, Bohdan