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

Fix caching Magento Metadata getVersion #26001

Merged
merged 3 commits into from Dec 18, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -13,9 +13,7 @@
use Magento\Framework\Composer\ComposerInformation;

/**
* Class ProductMetadata
*
* @package Magento\Framework\App
* Magento application product metadata
*/
class ProductMetadata implements ProductMetadataInterface
{
@@ -85,8 +83,8 @@ public function getVersion()
} else {
$this->version = 'UNKNOWN';
}
$this->cache->save($this->version, self::VERSION_CACHE_KEY, [Config::CACHE_TAG]);
}
$this->cache->save($this->version, self::VERSION_CACHE_KEY, [Config::CACHE_TAG]);
}
return $this->version;
}
@@ -5,6 +5,7 @@
*/
namespace Magento\Framework\App\Test\Unit;

use Magento\Framework\App\CacheInterface;
use Magento\Framework\App\ProductMetadata;
use Magento\Framework\TestFramework\Unit\Helper\ObjectManager;

@@ -20,13 +21,20 @@ class ProductMetadataTest extends \PHPUnit\Framework\TestCase
*/
private $composerInformationMock;

/**
* @var CacheInterface|\PHPUnit_Framework_MockObject_MockObject
*/
private $cacheMock;

protected function setUp()
{
$this->composerInformationMock = $this->getMockBuilder(\Magento\Framework\Composer\ComposerInformation::class)
->disableOriginalConstructor()->getMock();

$this->cacheMock = $this->getMockBuilder(CacheInterface::class)->getMock();

$objectManager = new ObjectManager($this);
$this->productMetadata = $objectManager->getObject(ProductMetadata::class);
$this->productMetadata = $objectManager->getObject(ProductMetadata::class, ['cache' => $this->cacheMock]);
$reflectionProperty = new \ReflectionProperty($this->productMetadata, 'composerInformation');
$reflectionProperty->setAccessible(true);
$reflectionProperty->setValue($this->productMetadata, $this->composerInformationMock);
@@ -40,11 +48,22 @@ protected function setUp()
public function testGetVersion($packageList, $expectedVersion)
{
$this->composerInformationMock->expects($this->any())->method('getSystemPackages')->willReturn($packageList);
$this->cacheMock->expects($this->once())->method('save')->with($expectedVersion);
$productVersion = $this->productMetadata->getVersion();
$this->assertNotEmpty($productVersion, 'Empty product version');
$this->assertEquals($expectedVersion, $productVersion);
}

public function testGetVersionCached()
This conversation was marked as resolved by ihor-sviziev

This comment has been minimized.

Copy link
@ihor-sviziev

ihor-sviziev Dec 13, 2019

Contributor

It not looks like caching is tested here, as you're not checking that save method was executed for cache mock.

Please update test

This comment has been minimized.

Copy link
@luklewluk

luklewluk Dec 13, 2019

Author Contributor

Updated. Please check now.

This comment has been minimized.

Copy link
@ihor-sviziev

ihor-sviziev Dec 13, 2019

Contributor

Seems like you expect that save method should be executed in some case. Issue was with cache saving, not with loading . Am I wrong ?

This comment has been minimized.

Copy link
@luklewluk

luklewluk Dec 13, 2019

Author Contributor

"save" method shouldn't be executed if value is loaded from cache. However I added it to testGetVersion test, because it's expected there to save the value from composer.

This comment has been minimized.

Copy link
@ihor-sviziev

ihor-sviziev Dec 13, 2019

Contributor

In the code you moved code that saves data to cache, and as for me - it's correct.
However in the test you covered another case - when we got some data - save will not be executed (and it wasn't executed even earlier) --> your test isn't covering the case that you've fixed.

This comment has been minimized.

Copy link
@luklewluk

luklewluk Dec 13, 2019

Author Contributor

I covered save execution in line 51. Method testGetVersionCached doesn't cover the fix indeed, it's just an additional test for loading from cache. Method testGetVersion covers the fix in line 51.

Do you want me to remove testGetVersionCached test then? I added it because I thought it's good to have better coverage, even if it wasn't a part of the fix.

This comment has been minimized.

Copy link
@ihor-sviziev

ihor-sviziev Dec 13, 2019

Contributor

Ah, sorry that’s my mistake, I missed it and thought you tried to cover it.

{
$expectedVersion = '1.2.3';
$this->composerInformationMock->expects($this->never())->method('getSystemPackages');
$this->cacheMock->expects($this->once())->method('load')->willReturn($expectedVersion);
$this->cacheMock->expects($this->never())->method('save');
$productVersion = $this->productMetadata->getVersion();
$this->assertEquals($expectedVersion, $productVersion);
}

/**
* @return array
*/
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.