Skip to content

Commit ccab4f5

Browse files
JacobBrownAustinJacobBrownAustin
committed
ACPT-1186
Using decorator for ObjectManagerFactory in order to better track objects created. In Collector, check if object's service name in sharedInstances of ObjectManager to compare it in skip list. In Comparator, Skipping objects that were previously skipped by Collector. Removed some dead code in Comparator. In Comparator, fixed bug with order of arguments to checkValues. Call _resetState on shared objects before and after each GraphQL request to make results more realistic. Needed to add more classes to state-skip-list.php .
1 parent b0708f6 commit ccab4f5

File tree

6 files changed

+132
-67
lines changed

6 files changed

+132
-67
lines changed

‎dev/tests/integration/testsuite/Magento/GraphQl/App/GraphQlStateTest.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@ class GraphQlStateTest extends \PHPUnit\Framework\TestCase
3434
/** @var ObjectManagerInterface */
3535
private ObjectManagerInterface $objectManagerBeforeTest;
3636

37-
/** @var ObjectManagerInterface */
38-
private ObjectManagerInterface $objectManagerForTest;
37+
/** @var ObjectManager */
38+
private ObjectManager $objectManagerForTest;
3939

4040
/** @var Comparator */
4141
private Comparator $comparator;
@@ -58,6 +58,7 @@ protected function setUp(): void
5858
$application->loadArea('graphql');
5959
$this->comparator = $this->objectManagerForTest->create(Comparator::class);
6060
$this->requestFactory = $this->objectManagerForTest->get(RequestFactory::class);
61+
$this->objectManagerForTest->resetStateSharedInstances();
6162
parent::setUp();
6263
}
6364

@@ -120,8 +121,10 @@ public function testState(
120121
*/
121122
private function request(string $query, string $operationName, bool $firstRequest = false): string
122123
{
124+
$this->objectManagerForTest->resetStateSharedInstances();
123125
$this->comparator->rememberObjectsStateBefore($firstRequest);
124126
$response = $this->doRequest($query);
127+
$this->objectManagerForTest->resetStateSharedInstances();
125128
$this->comparator->rememberObjectsStateAfter($firstRequest);
126129
$result = $this->comparator->compareBetweenRequests($operationName);
127130
$this->assertEmpty(

‎dev/tests/integration/testsuite/Magento/GraphQl/App/State/Collector.php‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ class Collector
1818
private array $skipListFromConstructed;
1919
private array $skipListBetweenRequests;
2020

21+
/** @var ObjectManager $objectManager Note: Using ObjectManagerInterface for DI to get correct instance */
22+
2123
public function __construct(
2224
private ObjectManagerInterface $objectManager,
2325
SkipListAndFilterList $skipListAndFilterList
@@ -82,13 +84,7 @@ function ($element) use (
8284
public function getSharedObjects(ShouldResetState $shouldResetState): array
8385
{
8486
$sharedObjects = [];
85-
$obj = new \ReflectionObject($this->objectManager);
86-
if (!$obj->hasProperty('_sharedInstances')) {
87-
throw new \Exception('Cannot get shared objects from ' . get_class($this->objectManager));
88-
}
89-
$property = $obj->getProperty('_sharedInstances');
90-
$property->setAccessible(true);
91-
foreach ($property->getValue($this->objectManager) as $serviceName => $object) {
87+
foreach ($this->objectManager->getSharedInstances() as $serviceName => $object) {
9288
if (array_key_exists($serviceName, $sharedObjects)) {
9389
continue;
9490
}
@@ -162,6 +158,10 @@ public function getPropertiesFromObject(
162158
if (array_key_exists($className, $skipList)) {
163159
return CollectedObject::getSkippedObject();
164160
}
161+
$serviceName = array_search($object, $this->objectManager->getSharedInstances());
162+
if ($serviceName && array_key_exists($serviceName, $skipList)) {
163+
return CollectedObject::getSkippedObject();
164+
}
165165
$objReflection = new \ReflectionObject($object);
166166
$properties = [];
167167
foreach ($objReflection->getProperties() as $property) {

‎dev/tests/integration/testsuite/Magento/GraphQl/App/State/Comparator.php‎

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -111,18 +111,6 @@ public function compareConstructedAgainstCurrent(string $operationName): array
111111
if (array_key_exists($className, $skipList)) {
112112
continue;
113113
}
114-
$propertiesToFilterList = [];
115-
if (isset($filterListServices[$className])) {
116-
$propertiesToFilterList[] = $filterListServices[$className];
117-
}
118-
foreach ($filterListParentClasses as $parentClass => $excludeProperties) {
119-
if ($object instanceof $parentClass) {
120-
$propertiesToFilterList[] = $excludeProperties;
121-
}
122-
}
123-
if ($filterListAll) {
124-
$propertiesToFilterList[] = $filterListAll;
125-
}
126114
$objectState = $this->compare($constructedObject, $currentObject, $skipList);
127115
if ($objectState) {
128116
$compareResults[$className] = $objectState;
@@ -147,6 +135,10 @@ private function compare(
147135
array $skipList,
148136
string $serviceName = '',
149137
) : array {
138+
$skippedObject = CollectedObject::getSkippedObject();
139+
if ($skippedObject === $before || $skippedObject === $after) {
140+
return []; // skipped
141+
}
150142
if (array_key_exists($before->getClassName(), $skipList)
151143
&& array_key_exists($after->getClassName(), $skipList)) {
152144
return []; // This object should be skipped
@@ -224,6 +216,10 @@ private function formatValue($value): mixed
224216
*/
225217
public function checkValues(mixed $before, mixed $after, array $skipList): array
226218
{
219+
$skippedObject = CollectedObject::getSkippedObject();
220+
if ($skippedObject === $before || $skippedObject === $after) {
221+
return []; // skipped
222+
}
227223
$typeBefore = gettype($before);
228224
$typeAfter = gettype($after);
229225
if ($typeBefore !== $typeAfter) {
@@ -245,19 +241,19 @@ public function checkValues(mixed $before, mixed $after, array $skipList): array
245241
if (count($before) !== count($after) || $before != $after) {
246242
$results = [];
247243
$keysChecked = [];
248-
foreach ($after as $key => $value) {
249-
$result = $this->checkValues($value, $before[$key] ?? null, $skipList);
244+
foreach ($after as $key => $valueAfter) {
245+
$result = $this->checkValues($before[$key] ?? null, $valueAfter, $skipList);
250246
if ($result) {
251247
$results[$key] = $result;
252248
}
253249
$keysChecked[$key] = true;
254250
}
255251
// Checking array keys that were in $before, but not $after
256-
foreach ($before as $key => $value) {
252+
foreach ($before as $key => $valueAfter) {
257253
if ($keysChecked[$key] ?? false) {
258254
continue;
259255
}
260-
$result = $this->checkValues($value, $before[$key] ?? null, $skipList);
256+
$result = $this->checkValues($before[$key] ?? null, $valueAfter, $skipList);
261257
if ($result) {
262258
$results[$key] = $result;
263259
}

‎dev/tests/integration/testsuite/Magento/GraphQl/App/State/DynamicFactoryDecorator.php‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
<?php
2+
/**
3+
* Copyright © Magento, Inc. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
declare(strict_types=1);
7+
8+
namespace Magento\GraphQl\App\State;
9+
10+
use Magento\Framework\ObjectManager\Factory\Dynamic\Developer;
11+
use Magento\Framework\ObjectManager\ResetAfterRequestInterface;
12+
use WeakMap;
13+
use WeakReference;
14+
15+
/**
16+
* Dynamic Factory Decorator for State test.
17+
*/
18+
class DynamicFactoryDecorator extends Developer implements ResetAfterRequestInterface
19+
{
20+
private WeakMap $weakMap; // values are CollectedObject
21+
private Collector $collector;
22+
private array $skipList;
23+
24+
public function __construct(Developer $developer, ObjectManager $objectManager)
25+
{
26+
/* Note: PHP doesn't have copy constructors, so we have to use get_object_vars,
27+
* but luckily all the properties in the superclass are protected. */
28+
$properties = get_object_vars($developer);
29+
foreach ($properties as $key => $value) {
30+
$this->$key = $value;
31+
}
32+
$this->objectManager = $objectManager;
33+
$this->weakMap = new WeakMap();
34+
$skipListAndFilterList = new SkipListAndFilterList;
35+
$this->skipList = $skipListAndFilterList->getSkipList('', CompareType::CompareConstructedAgainstCurrent);
36+
$this->collector = new Collector($this->objectManager, $skipListAndFilterList);
37+
$this->objectManager->addSharedInstance($skipListAndFilterList, SkipListAndFilterList::class);
38+
$this->objectManager->addSharedInstance($this->collector, Collector::class);
39+
}
40+
41+
/**
42+
* @inheritDoc
43+
*/
44+
public function create($type, array $arguments = [])
45+
{
46+
$object = parent::create($type, $arguments);
47+
if (!array_key_exists(get_class($object), $this->skipList)) {
48+
$this->weakMap[$object] =
49+
$this->collector->getPropertiesFromObject($object, CompareType::CompareConstructedAgainstCurrent);
50+
}
51+
return $object;
52+
}
53+
54+
/**
55+
* Reset state for all instances that we've created
56+
*
57+
* @return void
58+
* @SuppressWarnings(PHPMD.UnusedLocalVariable)
59+
*/
60+
public function _resetState(): void
61+
{
62+
/* Note: we can't iterate resetAfterWeakMap itself because it gets indirectly modified (shrinks) as some of the
63+
* service classes that get reset will destruct some of the other service objects. The iterator to WeakMap
64+
* returns actual objects, not WeakReferences. Therefore, we create a temporary list of weak references which
65+
* is safe to iterate. */
66+
$temporaryWeakReferenceList = [];
67+
foreach($this->weakMap as $weakMapObject => $value) {
68+
$temporaryWeakReferenceList[] = WeakReference::create($weakMapObject);
69+
unset($weakMapObject);
70+
unset($value);
71+
}
72+
foreach ($temporaryWeakReferenceList as $weakReference) {
73+
$object = $weakReference->get();
74+
if (!$object) {
75+
continue;
76+
}
77+
$object->_resetState();
78+
}
79+
}
80+
81+
public function getWeakMap() : WeakMap
82+
{
83+
return $this->weakMap;
84+
}
85+
}

‎dev/tests/integration/testsuite/Magento/GraphQl/App/State/ObjectManager.php‎

Lines changed: 17 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,7 @@
77

88
namespace Magento\GraphQl\App\State;
99

10-
use Magento\Framework\App\ObjectManager as AppObjectManager;
1110
use Magento\Framework\ObjectManager\ResetAfterRequestInterface;
12-
use Magento\TestFramework\Helper\Bootstrap;
1311
use Magento\TestFramework\ObjectManager as TestFrameworkObjectManager;
1412
use Weakmap;
1513

@@ -18,67 +16,44 @@
1816
*/
1917
class ObjectManager extends TestFrameworkObjectManager
2018
{
21-
private WeakMap $weakMap;
22-
private Collector $collector;
23-
private array $skipList;
24-
2519
/**
2620
* Constructs this instance by copying test framework's ObjectManager
2721
*
2822
* @param TestFrameworkObjectManager $testFrameworkObjectManager
2923
*/
3024
public function __construct(TestFrameworkObjectManager $testFrameworkObjectManager)
3125
{
32-
$this->weakMap = new WeakMap();
3326
/* Note: PHP doesn't have copy constructors, so we have to use get_object_vars,
3427
* but luckily all the properties in the superclass are protected. */
3528
$properties = get_object_vars($testFrameworkObjectManager);
3629
foreach ($properties as $key => $value) {
3730
$this->$key = $value;
3831
}
39-
$skipListAndFilterList = new SkipListAndFilterList;
40-
$this->skipList = $skipListAndFilterList->getSkipList('', CompareType::CompareConstructedAgainstCurrent);
41-
$this->collector = new Collector($this, $skipListAndFilterList);
42-
$this->_sharedInstances[SkipListAndFilterList::class] = $skipListAndFilterList;
43-
$this->_sharedInstances[Collector::class] = $this->collector;
32+
$this->_factory = new DynamicFactoryDecorator($this->_factory, $this);
4433
}
4534

46-
/**
47-
* @inheritDoc
48-
*/
49-
public function create($type, array $arguments = [])
35+
public function getWeakMap() : WeakMap
5036
{
51-
$object = parent::create($type, $arguments);
52-
if (!array_key_exists(get_class($object), $this->skipList)) {
53-
$this->weakMap[$object] =
54-
$this->collector->getPropertiesFromObject($object, CompareType::CompareConstructedAgainstCurrent);
55-
}
56-
return $object;
37+
return $this->_factory->getWeakMap();
5738
}
5839

59-
/**
60-
* @inheritDoc
61-
*/
62-
public function get($requestedType)
40+
public function getSharedInstances() : array
6341
{
64-
$object = parent::get($requestedType);
65-
if (null === ($this->weakMap[$object] ?? null)) {
66-
if (!array_key_exists(get_class($object), $this->skipList)) {
67-
if ($object instanceof ResetAfterRequestInterface) {
68-
/* Note: some service classes get added to weakMap after they are already used, so
69-
* we need to make sure to reset them to get proper initial state after construction for
70-
* comparison */
71-
$object->_resetState();
72-
}
73-
$this->weakMap[$object] =
74-
$this->collector->getPropertiesFromObject($object, CompareType::CompareConstructedAgainstCurrent);
75-
}
76-
}
77-
return $object;
42+
return $this->_sharedInstances;
7843
}
7944

80-
public function getWeakMap() : WeakMap
45+
public function resetStateWeakMapObjects() : void
8146
{
82-
return $this->weakMap;
47+
$this->_factory->_resetState();
48+
}
49+
50+
public function resetStateSharedInstances() : void
51+
{
52+
/** @var object $object */
53+
foreach ($this->_sharedInstances as $object) {
54+
if ($object instanceof ResetAfterRequestInterface) {
55+
$object->_resetState();
56+
}
57+
}
8358
}
8459
}

‎dev/tests/integration/testsuite/Magento/GraphQl/_files/state-skip-list.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,9 @@
9090
Magento\Framework\ObjectManager\DefinitionInterface::class => null,
9191
Magento\TestFramework\App\State::class => null,
9292
Magento\GraphQl\App\State\SkipListAndFilterList::class => null, // Yes, our test uses mutable state itself :-)
93+
Magento\Framework\App\ResourceConnection::class => null,
94+
Magento\Framework\App\ResourceConnection\Interceptor::class => null,
95+
Magento\Framework\Session\SaveHandler::class => null, // TODO: check this
9396
],
9497
'*-fromConstructed' => [
9598
Magento\GraphQl\App\State\ObjectManager::class => null,
@@ -183,6 +186,9 @@
183186
Magento\Framework\Module\Manager::class => null,
184187
Magento\Eav\Api\Data\AttributeExtension::class => null, // FIXME: This needs to be fixed. is_pagebuilder_enabled 0 => null
185188
Magento\TestFramework\Event\Magento::class => null,
189+
Magento\Staging\Model\VersionManager\Interceptor::class => null, // Has good _resetState
190+
Magento\Webapi\Model\Authorization\TokenUserContext::class => null, // Has good _resetState
191+
Magento\Store\Model\Website\Interceptor::class => null, // reset by poison pill
186192
],
187193
'' => [
188194
],

0 commit comments

Comments
 (0)