Skip to content

33309: Eliminated AspectMock usage from TestGeneratorTest.php #850

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

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,18 @@
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace tests\unit\Magento\FunctionalTestFramework\Util;

use AspectMock\Test as AspectMock;

use Exception;
use Magento\FunctionalTestingFramework\Exceptions\TestReferenceException;
use Magento\FunctionalTestingFramework\Filter\FilterList;
use Magento\FunctionalTestingFramework\Test\Handlers\TestObjectHandler;
use Magento\FunctionalTestingFramework\Test\Objects\ActionObject;
use Magento\FunctionalTestingFramework\Test\Objects\TestHookObject;
use Magento\FunctionalTestingFramework\Test\Objects\TestObject;
use Magento\FunctionalTestingFramework\Util\Filesystem\CestFileCreatorUtil;
use ReflectionProperty;
use tests\unit\Util\MagentoTestCase;
use Magento\FunctionalTestingFramework\Util\TestGenerator;
use Magento\FunctionalTestingFramework\Config\MftfApplicationConfig;
Expand All @@ -22,43 +24,37 @@
class TestGeneratorTest extends MagentoTestCase
{
/**
* Before method functionality
* Before method functionality.
*/
public function setUp(): void
{
TestLoggingUtil::getInstance()->setMockLoggingUtil();
}

/**
* After method functionality
* After method functionality.
*
* @return void
*/
public function tearDown(): void
{
AspectMock::clean();
GenerationErrorHandler::getInstance()->reset();
}

/**
* Basic test to check exceptions for incorrect entities.
*
* @throws \Exception
* @return void
* @throws Exception
*/
public function testEntityException()
public function testEntityException(): void
{
$actionObject = new ActionObject('fakeAction', 'comment', [
'userInput' => '{{someEntity.entity}}'
]);

$testObject = new TestObject("sampleTest", ["merge123" => $actionObject], [], [], "filename");

AspectMock::double(TestObjectHandler::class, ['initTestData' => '']);

$testGeneratorObject = TestGenerator::getInstance("", ["sampleTest" => $testObject]);

AspectMock::double(TestGenerator::class, ['loadAllTestObjects' => ["sampleTest" => $testObject]]);

$testGeneratorObject->createAllTestFiles(null, []);

// assert that no exception for createAllTestFiles and generation error is stored in GenerationErrorHandler
Expand All @@ -69,11 +65,12 @@ public function testEntityException()
}

/**
* Tests that skipped tests do not have a fully generated body
* Tests that skipped tests do not have a fully generated body.
*
* @throws \Magento\FunctionalTestingFramework\Exceptions\TestReferenceException
* @return void
* @throws TestReferenceException
*/
public function testSkippedNoGeneration()
public function testSkippedNoGeneration(): void
{
$actionInput = 'fakeInput';
$actionObject = new ActionObject('fakeAction', 'comment', [
Expand All @@ -91,14 +88,22 @@ public function testSkippedNoGeneration()
}

/**
* Tests that skipped tests have a fully generated body when --allowSkipped is passed in
* Tests that skipped tests have a fully generated body when --allowSkipped is passed in.
*
* @throws \Magento\FunctionalTestingFramework\Exceptions\TestReferenceException
* @return void
* @throws TestReferenceException
*/
public function testAllowSkipped()
public function testAllowSkipped(): void
{
// Mock allowSkipped for TestGenerator
AspectMock::double(MftfApplicationConfig::class, ['allowSkipped' => true]);
$mockConfig = $this->createMock(MftfApplicationConfig::class);
$mockConfig->expects($this->any())
->method('allowSkipped')
->willReturn(true);

$property = new ReflectionProperty(MftfApplicationConfig::class, 'MFTF_APPLICATION_CONTEXT');
$property->setAccessible(true);
$property->setValue($mockConfig);

$actionInput = 'fakeInput';
$actionObject = new ActionObject('fakeAction', 'comment', [
Expand Down Expand Up @@ -128,17 +133,20 @@ public function testAllowSkipped()
}

/**
* Tests that TestGenerator createAllTestFiles correctly filters based on severity
* Tests that TestGenerator createAllTestFiles correctly filters based on severity.
*
* @throws \Magento\FunctionalTestingFramework\Exceptions\TestReferenceException
* @return void
* @throws TestReferenceException
*/
public function testFilter()
public function testFilter(): void
{
// Mock filters for TestGenerator
AspectMock::double(
MftfApplicationConfig::class,
['getFilterList' => new FilterList(['severity' => ["CRITICAL"]])]
);
$mockConfig = $this->createMock(MftfApplicationConfig::class);
$fileList = new FilterList(['severity' => ["CRITICAL"]]);
$mockConfig->expects($this->once())->method('getFilterList')->willReturn($fileList);

$property = new ReflectionProperty(MftfApplicationConfig::class, 'MFTF_APPLICATION_CONTEXT');
$property->setAccessible(true);
$property->setValue($mockConfig);

$actionInput = 'fakeInput';
$actionObject = new ActionObject('fakeAction', 'comment', [
Expand All @@ -161,16 +169,26 @@ public function testFilter()
[],
"filename"
);
AspectMock::double(TestGenerator::class, ['loadAllTestObjects' => ["sampleTest" => $test1, "test2" => $test2]]);

// Mock createCestFile to return name of tests that testGenerator tried to create
$generatedTests = [];
AspectMock::double(TestGenerator::class, ['createCestFile' => function ($arg1, $arg2) use (&$generatedTests) {
$generatedTests[$arg2] = true;
}]);
$cestFileCreatorUtil = $this->createMock(CestFileCreatorUtil::class);
$cestFileCreatorUtil->expects($this->once())
->method('create')
->will(
$this->returnCallback(
function ($filename) use (&$generatedTests) {
$generatedTests[$filename] = true;
}
)
);

$property = new ReflectionProperty(CestFileCreatorUtil::class, 'INSTANCE');
$property->setAccessible(true);
$property->setValue($cestFileCreatorUtil);

$testGeneratorObject = TestGenerator::getInstance("", ["sampleTest" => $test1, "test2" => $test2]);
$testGeneratorObject->createAllTestFiles(null, []);
$testGeneratorObject->createAllTestFiles();

// Ensure Test1 was Generated but not Test 2
$this->assertArrayHasKey('test1Cest', $generatedTests);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\FunctionalTestingFramework\Util\Filesystem;

use Magento\FunctionalTestingFramework\Exceptions\TestFrameworkException;

class CestFileCreatorUtil
{
/**
* Singleton CestFileCreatorUtil Instance.
*
* @var CestFileCreatorUtil
*/
private static $INSTANCE;

/**
* CestFileCreatorUtil constructor.
*/
private function __construct()
{
}

/**
* Get CestFileCreatorUtil instance.
*
* @return CestFileCreatorUtil
*/
public static function getInstance()
{
if (self::$INSTANCE === null) {
return new self();
}

return self::$INSTANCE;
}

/**
* Create a single PHP file containing the $cestPhp using the $filename.
* If the _generated directory doesn't exist it will be created.
*
* @param string $filename
* @param string $exportDirectory
* @param string $testPhp
*
* @return void
* @throws TestFrameworkException
*/
public function create(string $filename, string $exportDirectory, string $testPhp): void
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of pulling some code out of TestGenerator, but I don't think that creating a singleton that hold no data for a single utility function makes sense.

I would sooner that they change the visibility of createCestFile to public and mock that method instead. This is a lot of code to introduce to mock a single method.

Copy link
Contributor

@bohdan-harniuk bohdan-harniuk Jul 29, 2021

Choose a reason for hiding this comment

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

@KevinBKozan, as for me, in general, the composition is much better than big classes with a variety of functions.
@jilu1, what do you think about that concrete case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@bohdan-harniuk Yeah, I see your point in that. I'm just vigilant about creating any classes that will perpetually live in memory, but I suppose that's not much of a concern if there's not a large amount of data being held.

I'm happy to defer to @jilu1 's original review in this case, I'll approve this and we can merge it.

{
DirSetupUtil::createGroupDir($exportDirectory);
$exportFilePath = $exportDirectory . DIRECTORY_SEPARATOR . $filename . '.php';
$file = fopen($exportFilePath, 'w');

if (!$file) {
throw new TestFrameworkException(
sprintf('Could not open test file: "%s"', $exportFilePath)
);
}

fwrite($file, $testPhp);
fclose($file);
}
}
13 changes: 3 additions & 10 deletions src/Magento/FunctionalTestingFramework/Util/TestGenerator.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use Magento\FunctionalTestingFramework\Test\Objects\TestHookObject;
use Magento\FunctionalTestingFramework\Test\Objects\TestObject;
use Magento\FunctionalTestingFramework\Test\Util\BaseObjectExtractor;
use Magento\FunctionalTestingFramework\Util\Filesystem\CestFileCreatorUtil;
use Magento\FunctionalTestingFramework\Util\Logger\LoggingUtil;
use Magento\FunctionalTestingFramework\Util\Manifest\BaseTestManifest;
use Magento\FunctionalTestingFramework\Test\Util\ActionObjectExtractor;
Expand Down Expand Up @@ -207,21 +208,13 @@ private function loadAllTestObjects($testsToIgnore)
*
* @param string $testPhp
* @param string $filename
*
* @return void
* @throws TestFrameworkException
*/
private function createCestFile(string $testPhp, string $filename)
{
DirSetupUtil::createGroupDir($this->exportDirectory);
$exportFilePath = $this->exportDirectory . DIRECTORY_SEPARATOR . $filename . ".php";
$file = fopen($exportFilePath, 'w');

if (!$file) {
throw new TestFrameworkException(sprintf('Could not open test file: "%s"', $exportFilePath));
}

fwrite($file, $testPhp);
fclose($file);
CestFileCreatorUtil::getInstance()->create($filename, $this->exportDirectory, $testPhp);
}

/**
Expand Down