Skip to content
This repository has been archived by the owner on Apr 29, 2019. It is now read-only.

Added $websiteString property to test class #142

Merged
merged 2 commits into from
Nov 6, 2018
Merged

Added $websiteString property to test class #142

merged 2 commits into from
Nov 6, 2018

Conversation

maxalmonte14
Copy link
Contributor

@maxalmonte14 maxalmonte14 commented Oct 26, 2018

Description (*)

This PR solves the problem addressed in #132

Fixed Issues (if relevant)

  1. Clean up app/code/Magento/AdvancedPricingImportExport/Test/Unit/Model/Import/AdvancedPricing/Validator/WebsiteTest.php #132:
    Clean up app/code/Magento/AdvancedPricingImportExport/Test/Unit/Model/Import/AdvancedPricing/Validator/WebsiteTest.php

Manual testing scenarios (*)

  1. Run ./vendor/bin/phpstan analyse app/code/Magento/AdvancedPricingImportExport/Test/Unit/Model/Import/AdvancedPricing/Validator/WebsiteTest.phpas described in Clean up app/code/Magento/AdvancedPricingImportExport/Test/Unit/Model/Import/AdvancedPricing/Validator/WebsiteTest.php #132, after this implementation the following error shouldn't be present
------ -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   WebsiteTest.php                                                                                                                                                                     
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  106    Access to an undefined property Magento\AdvancedPricingImportExport\Test\Unit\Model\Import\AdvancedPricing\Validator\WebsiteTest::$websiteString.                                   
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

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/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@dmanners
Copy link
Contributor

Hello, thank you for your pull request. I will start to process this PR and get back to you if I need any more information.

/**
* @var \Magento\AdvancedPricingImportExport\Model\Import\AdvancedPricing\Validator\Website|\PHPUnit_Framework_MockObject_MockObject
*/
protected $websiteString;
Copy link
Contributor

Choose a reason for hiding this comment

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

After looking into the usage of this it seems to only be used in one method. I would suggest in this case we convert it to a local variable to the function. Something like:

        $websiteString = $this->getMockBuilder(
            \Magento\AdvancedPricingImportExport\Model\Import\AdvancedPricing\Validator\Website::class
        )
            ->setMethods(['_clearMessages', '_addMessages'])
            ->setConstructorArgs([$this->storeResolver, $this->webSiteModel])
            ->getMock();
        $result = $websiteString->getAllWebsitesValue();

After making this change please also run the unit test to make sure it is not failing. This can be done via the command:

./vendor/bin/phpunit -c dev/tests/unit/phpunit.xml.dist app/code/Magento/AdvancedPricingImportExport/Test/Unit/Model/Import/AdvancedPricing/Validator/WebsiteTest.php

@maxalmonte14
Copy link
Contributor Author

I extracted the $websiteString to a local variable inside the testGetAllWebsitesValue method, also I made sure the test is passing by running ./vendor/bin/phpunit -c dev/tests/unit/phpunit.xml.dist app/code/Magento/AdvancedPricingImportExport/Test/Unit/Model/Import/AdvancedPricing/Validator/WebsiteTest.php 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants