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.

Sign up for GitHub

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

Jump to bottom

MNT Fix unit tests #11432

Merged
merged 1 commit into from
Oct 22, 2024
Merged

MNT Fix unit tests #11432

merged 1 commit into from
Oct 22, 2024

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Oct 21, 2024 •
edited
Loading

Issue silverstripe/.github#320

Fixes a few unit tests

@emteknetnz emteknetnz mentioned this pull request Oct 21, 2024
Closed
@emteknetnz emteknetnz force-pushed the pulls/6/fix-definemethods branch 2 times, most recently from 51abdd0 to ba5f55e Compare October 21, 2024 03:22
@emteknetnz emteknetnz force-pushed the pulls/6/fix-definemethods branch 2 times, most recently from 63770ec to 2ae9443 Compare October 21, 2024 05:22
emteknetnz
emteknetnz commented Oct 21, 2024
View reviewed changes
tests/php/ORM/Search/SearchContextTest.php Outdated
@@ -138,16 +138,36 @@ public function testUserDefinedFiltersAppearInSearchContext()
public function testUserDefinedFieldsAppearInSearchContext()
{
$company = SearchContextTest\Company::singleton();
$searchName = $company->getGeneralSearchFieldName();
Copy link
Member Author

@emteknetnz emteknetnz Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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' => [...]`

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@emteknetnz emteknetnz Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Have changed solution

GuySartorelli
GuySartorelli requested changes Oct 21, 2024
View reviewed changes
src/Core/CustomMethods.php Outdated
Comment on lines 174 to 177
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])
) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@emteknetnz emteknetnz Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'll split this off as a separate PR so the others can be merged

tests/php/ORM/Search/SearchContextTest.php Outdated
@@ -138,16 +138,36 @@ public function testUserDefinedFiltersAppearInSearchContext()
public function testUserDefinedFieldsAppearInSearchContext()
{
$company = SearchContextTest\Company::singleton();
$searchName = $company->getGeneralSearchFieldName();
Copy link
Member

Choose a reason for hiding this comment

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

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 emteknetnz changed the title FIX Call defineMethods if lower method is missing MNT Fix unit tests Oct 21, 2024
@emteknetnz emteknetnz marked this pull request as draft October 21, 2024 23:47
@emteknetnz
Copy link
Member Author

Failing unit test is in silverstripe/cms and is fixed in silverstripe/silverstripe-cms#3018

@emteknetnz emteknetnz marked this pull request as ready for review October 22, 2024 02:43
emteknetnz
emteknetnz commented Oct 22, 2024
View reviewed changes
src/Core/Extensible.php
@@ -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)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

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' => [...]

GuySartorelli
GuySartorelli approved these changes Oct 22, 2024
View reviewed changes
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM

@GuySartorelli GuySartorelli merged commit 662ac9d into silverstripe:6 Oct 22, 2024
8 of 10 checks passed
@GuySartorelli GuySartorelli deleted the pulls/6/fix-definemethods branch October 22, 2024 03:25
@GuySartorelli GuySartorelli mentioned this pull request Oct 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuySartorelli GuySartorelli GuySartorelli approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@emteknetnz @GuySartorelli