MNT Fix unit tests #11432
MNT Fix unit tests #11432
Conversation
emteknetnz
force-pushed
the
pulls/6/fix-definemethods
branch
2 times, most recently
from
51abdd0 to
ba5f55e
Compare
emteknetnz
force-pushed
the
pulls/6/fix-definemethods
branch
2 times, most recently
from
63770ec to
2ae9443
Compare
| @@ -138,16 +138,36 @@ public function testUserDefinedFiltersAppearInSearchContext() | |||
| public function testUserDefinedFieldsAppearInSearchContext() | |||
| { | |||
| $company = SearchContextTest\Company::singleton(); | |||
| $searchName = $company->getGeneralSearchFieldName(); | |||
There was a problem hiding this comment.
Fixes issue
1) SilverStripe\ORM\Tests\Search\SearchContextTest::testUserDefinedFieldsAppearInSearchContext
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
'failover' => null
'customisedObject' => null
'objCache' => []
- 'extension_instances' => null
+ 'extension_instances' => [...]`
There was a problem hiding this comment.
It's hard to tell what you've actually changed here, other than adding a whole load of assertions that weren't there before.
What was the root cause here and what specific change fixes it, vs just adding new tests?
There was a problem hiding this comment.
I don't know what the root cause is, there have been a lot of things merged into CMS 6 recently. I'm not sure if this is an actual issue.
This specific unit test doesn't care about extension_instances, what it's actually testing is that the scaffolded search fields match Company.$searchable_fields
There was a problem hiding this comment.
Have changed solution
emteknetnz
force-pushed
the
pulls/6/fix-definemethods
branch
from
2ae9443 to
f180723
Compare
src/Core/CustomMethods.php
Outdated
| if (!isset(self::class::$extra_methods[$lowerClass])) { | ||
| $lowerMethod = strtolower($method); | ||
| if (!array_key_exists($lowerClass, self::class::$extra_methods) | ||
| || !array_key_exists($lowerMethod, self::class::$extra_methods[$lowerClass]) | ||
| ) { |
There was a problem hiding this comment.
This feels like it's masking a regression rather than really fixing the problem.
This condition is intended to result in calling defineMethods() any time it hasn't been done yet for this class.
The change will run the defineMethods() code any time a method doesn't exist, regardless of whether defineMethods() has already been run for this class.
There was a problem hiding this comment.
I'll split this off as a separate PR so the others can be merged
| @@ -138,16 +138,36 @@ public function testUserDefinedFiltersAppearInSearchContext() | |||
| public function testUserDefinedFieldsAppearInSearchContext() | |||
| { | |||
| $company = SearchContextTest\Company::singleton(); | |||
| $searchName = $company->getGeneralSearchFieldName(); | |||
There was a problem hiding this comment.
It's hard to tell what you've actually changed here, other than adding a whole load of assertions that weren't there before.
What was the root cause here and what specific change fixes it, vs just adding new tests?
emteknetnz
changed the title
emteknetnz
force-pushed
the
pulls/6/fix-definemethods
branch
from
f180723 to
790faa6
Compare
emteknetnz
force-pushed
the
pulls/6/fix-definemethods
branch
from
790faa6 to
f5bc934
Compare
|
Failing unit test is in silverstripe/cms and is fixed in silverstripe/silverstripe-cms#3018 |
emteknetnz
force-pushed
the
pulls/6/fix-definemethods
branch
from
f5bc934 to
983e90b
Compare
| @@ -511,7 +511,7 @@ public function getExtensionInstances() | |||
| // Setup all extension instances for this instance | |||
| $this->extension_instances = []; | |||
| foreach (ClassInfo::ancestry(static::class) as $class) { | |||
| if (in_array($class, self::$unextendable_classes)) { | |||
| if (in_array($class, self::class::$unextendable_classes)) { | |||
There was a problem hiding this comment.
Should be self::class as per https://docs.silverstripe.org/en/5/contributing/coding_conventions/php_coding_conventions/#other-conventions
Fixes issue
1) SilverStripe\ORM\Tests\Search\SearchContextTest::testUserDefinedFieldsAppearInSearchContext
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
'failover' => null
'customisedObject' => null
'objCache' => []
- 'extension_instances' => null
+ 'extension_instances' => [...]
Issue silverstripe/.github#320
Fixes a few unit tests