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

Unable to run \Magento\Downloadable\Test\Unit\Helper\DownloadTest without internet connection / dns resolution #23521

Closed
adrian-martinez-interactiv4 opened this issue Jul 1, 2019 · 4 comments
Assignees
Labels
Component: Test Fixed in 2.4.x The issue has been fixed in 2.4-develop branch Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release

Comments

@adrian-martinez-interactiv4
Copy link
Contributor

adrian-martinez-interactiv4 commented Jul 1, 2019

Preconditions (*)

  1. Magento 2.3.2, just created project, no setup:install
  2. PHP 7.2.19

Steps to reproduce (*)

  1. Create Magento 2.3.2 project
  2. Run unit tests (at least \Magento\Downloadable\Test\Unit\Helper\DownloadTest) without internet connection

Expected result (*)

  1. Test should pass (image below shows test execution with internet connection):

Captura de pantalla 2019-07-01 a las 13 04 04

Actual result (*)

  1. Test fails with message (image below shows test execution without internet connection): PHPUnit\Framework\Exception: Warning: get_headers(): php_network_getaddresses: getaddrinfo failed: nodename nor servname provided, or not known in /Users/adrianmartinez/Sites/2.3/magento/vendor/magento/module-downloadable/Helper/Download.php:264.

Captura de pantalla 2019-07-01 a las 13 04 58

Extended description

This behaviour cannot be reproduced in 2.2.8, test passes even if there is no internet connection. Comparing code both versions, I've seen this snippet of code has been added in new version of tested class \Magento\Downloadable\Helper\Download:

    public function setResource($resourceFile, $linkType = self::LINK_TYPE_FILE)
    {
        (...)
        
        /**
        * check header for urls
        */
        if ($linkType === self::LINK_TYPE_URL) {
            $headers = array_change_key_case(get_headers($this->_resourceFile, 1), CASE_LOWER);
            if (isset($headers['location'])) {
                $this->_resourceFile  = is_array($headers['location']) ? current($headers['location'])
                    : $headers['location'];
            }
        }
        
        (...)
    }

This new code looks quite risky and not safe, I mean, even if problem is the test not passing, this piece of code has some assumptions that can break execution at any time.

First of all, test relies on a test url: \Magento\Downloadable\Test\Unit\Helper\DownloadTest::URL = 'http://example.com', that may exist or not, or could be down at any time. This means this test could even fail with internet connection if that site is down or DNS servers are not available.

In second place, it tries to extract headers using get_headers php built-in function, and passes the result directly to array_change_key_case, without any kind of check. get_headers may emit a warning if it is not able to get header of supplied url, but even if silenced with @get_headers, execution throws another warning due to get_headers returning false instead of an array, and array_change_key_case complaining about receiving a boolean parameter instead of an array:

1) Magento\Downloadable\Test\Unit\Helper\DownloadTest::testGetFileSizeUrl
PHPUnit\Framework\Exception: Warning: array_change_key_case() expects parameter 1 to be array, boolean given in /Users/adrianmartinez/Sites/2.3/magento/vendor/magento/module-downloadable/Helper/Download.php:264.

I think that piece of code should be refactored through a HeadersResolver (or another more appropriate name), responsible of returning always a consistent return value when requested for some headers (always return an array), and use this new class in \Magento\Downloadable\Helper\Download, so it can be mocked for testing.

@m2-assistant
Copy link

m2-assistant bot commented Jul 1, 2019

Hi @adrian-martinez-interactiv4. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

  • Summary of the issue
  • Information on your environment
  • Steps to reproduce
  • Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento give me 2.3-develop instance - upcoming 2.3.x release

For more details, please, review the Magento Contributor Assistant documentation.

@adrian-martinez-interactiv4 do you confirm that you were able to reproduce the issue on vanilla Magento instance following steps to reproduce?

  • yes
  • no

@magento-engcom-team magento-engcom-team added the Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed label Jul 1, 2019
@engcom-Charlie engcom-Charlie self-assigned this Jul 2, 2019
@engcom-Charlie engcom-Charlie added Component: Test Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Jul 2, 2019
@magento-engcom-team magento-engcom-team added the Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development label Jul 2, 2019
@magento-engcom-team
Copy link
Contributor

✅ Confirmed by @engcom-Charlie
Thank you for verifying the issue. Based on the provided information internal tickets MC-17920 were created

Issue Available: @engcom-Charlie, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

@aleromano89
Copy link
Contributor

@magento-engcom-team the issue also happens on 2.4-develop

aleromano89 added a commit to aleromano89/magento2 that referenced this issue Jan 4, 2020
@aleromano89 aleromano89 mentioned this issue Jan 4, 2020
4 tasks
@ghost ghost assigned aleromano89 Jan 4, 2020
@VladimirZaets VladimirZaets added the Fixed in 2.4.x The issue has been fixed in 2.4-develop branch label Jan 8, 2020
@VladimirZaets
Copy link
Contributor

Hi @adrian-martinez-interactiv4. Thank you for your report.
The issue has been fixed in #26264 by @aleromano89 in 2.4-develop branch
Related commit(s):

The fix will be available with the upcoming 2.4.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Test Fixed in 2.4.x The issue has been fixed in 2.4-develop branch Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

No branches or pull requests

5 participants