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

bohdan-harniuk commented 3 years ago

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):

_parseurl() 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

magento/magento2/issues/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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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

magento-engcom-team commented 3 years ago

@jilu1 the pull request successfully imported.

bohdan-harniuk commented 3 years ago

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

magento / magento2-functional-testing-framework