Skip to content

[TASK] Add tests for RuleSet::getRulesAssoc with $searchPattern #1280

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

Merged
oliverklee merged 4 commits into from
Jun 21, 2025
Merged

[TASK] Add tests for RuleSet::getRulesAssoc with $searchPattern #1280

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b7c90b1
[TASK] Add tests for `RuleSet::getRulesAssoc` with `$searchPattern`
JakeQZ Jun 19, 2025
8c7419c
Add separate test for non-matching patterns
JakeQZ Jun 19, 2025
a32a698
Use `assertArrayHasKey()`
JakeQZ Jun 19, 2025
fcca94e
Use single test method check that exactly all property names are matched
JakeQZ Jun 20, 2025
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
78 changes: 78 additions & 0 deletions tests/Unit/RuleSet/RuleSetTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1165,6 +1165,84 @@ public function getRulesAssocKeysRulesByPropertyName(): void
}
}

/**
* @test
*
* @param list<string> $propertyNamesToSet
* @param list<string> $matchingPropertyNames
*
* @dataProvider providePropertyNamesAndSearchPatternAndMatchingPropertyNames
*/
public function getRulesAssocWithPatternReturnsAllMatchingPropertyNames(
array $propertyNamesToSet,
string $searchPattern,
array $matchingPropertyNames
): void {
$this->setRulesFromPropertyNames($propertyNamesToSet);

$result = $this->subject->getRulesAssoc($searchPattern);

foreach ($matchingPropertyNames as $expectedMatchingPropertyName) {
self::assertContains($expectedMatchingPropertyName, \array_keys($result));
Copy link
Collaborator

@oliverklee oliverklee Jun 19, 2025

Choose a reason for hiding this comment

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

If I understand this code correctly, we can use assertArrayHasKey here.

https://docs.phpunit.de/en/9.6/assertions.html#assertarrayhaskey

Copy link
Collaborator Author

@JakeQZ JakeQZ Jun 19, 2025

Choose a reason for hiding this comment

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

Yes. I didn't think of that.

}
}

/**
* @test
*
* @param list<string> $propertyNamesToSet
* @param list<string> $matchingPropertyNames
*
* @dataProvider providePropertyNamesAndSearchPatternAndMatchingPropertyNames
*/
public function getRulesAssocWithPatternFiltersNonMatchingRules(
array $propertyNamesToSet,
string $searchPattern,
array $matchingPropertyNames
): void {
$this->setRulesFromPropertyNames($propertyNamesToSet);

$result = $this->subject->getRulesAssoc($searchPattern);

foreach ($result as $resultRule) {
Copy link
Collaborator

@oliverklee oliverklee Jun 19, 2025

Choose a reason for hiding this comment

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

I have a really hard time wrapping my head around what this test does … maybe some class in our project needs some well-named convenience methods to check for things? What do you think=

Copy link
Collaborator Author

@JakeQZ JakeQZ Jun 19, 2025

Choose a reason for hiding this comment

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

It is the converse of the previous test, which asserts that all matching Rules are returned. This test asserts that no non-matching Rules are returned.

Perhaps the test could be better named, or more clearly written. Or perhaps these two tests could be reduced to a single test that asserts the set of Rules returned is as expected.

For getRules(), I also created two separate tests, one asserting that all matching Rules were returned, and one asserting that no non-matching Rules were returned.

For getRulesAssoc() there is the added complication that only one Rule per property name is returned (as documented in the DocBlock), which means the expected result may not contain all matching Rules if they have the same property name.

Note that Rule::getRule() returns the property name and is a misnomer.

Copy link
Collaborator Author

@JakeQZ JakeQZ Jun 19, 2025

Choose a reason for hiding this comment

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

Perhaps getRulesAssocWithPatternReturnsOnlyMatchingRules() would be a clearer test method name.

Copy link
Collaborator Author

@JakeQZ JakeQZ Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Basically, this test asserts that every Rule in the result has a property name in $matchingPropertyNames. It would be nice to avoid tranposing the meanings of actual and expected in the assertions, but I could not think of a way of doing so.

In this test we could bypass Rule::getRule() and test the array keys in the result instead, which should be the same (that they will be is covered by other tests).

Copy link
Collaborator

@oliverklee oliverklee Jun 20, 2025

Choose a reason for hiding this comment

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

Basically, this test asserts that every Rule in the result has a property name in $matchingPropertyNames

Would it work to compare (sorted) arrays instead so we can drop the loop?

In this test we could bypass Rule::getRule() and test the array keys in the result instead

Yes, I think that would be easier to grok. Let's do this!

Copy link
Collaborator

@oliverklee oliverklee Jun 20, 2025

Choose a reason for hiding this comment

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

Perhaps getRulesAssocWithPatternReturnsOnlyMatchingRules() would be a clearer test method name.

I concur.

Copy link
Collaborator

@oliverklee oliverklee Jun 20, 2025

Choose a reason for hiding this comment

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

Perhaps the test could be better named, or more clearly written. Or perhaps these two tests could be reduced to a single test that asserts the set of Rules returned is as expected.

Yes, I'd prefer that.

Copy link
Collaborator Author

@JakeQZ JakeQZ Jun 20, 2025

Choose a reason for hiding this comment

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

Or perhaps these two tests could be reduced to a single test that asserts the set of Rules returned is as expected.

Yes, I'd prefer that.

I've modified getRulesAssocWithPatternReturnsAllMatchingPropertyNames() to assert that the list of property names returned exactly matches those expected, so it now also ensures no non-matching ones are returned. (I haven't changed that test method name.)

Would it work to compare (sorted) arrays instead so we can drop the loop?

Done.

In this test we could bypass Rule::getRule() and test the array keys in the result instead

Yes, I think that would be easier to grok. Let's do this!

Done.

Perhaps getRulesAssocWithPatternReturnsOnlyMatchingRules() would be a clearer test method name.

I concur.

This test has been removed, since it's now covered by the other test.

// 'expected' and 'actual' are transposed here due to necessity
self::assertContains($resultRule->getRule(), $matchingPropertyNames);
}
}

/**
* @test
*
* @param list<string> $propertyNamesToSet
*
* @dataProvider providePropertyNamesAndNonMatchingSearchPattern
*/
public function getRulesAssocWithNonMatchingPatternReturnsEmptyArray(
array $propertyNamesToSet,
string $searchPattern
): void {
$this->setRulesFromPropertyNames($propertyNamesToSet);

$result = $this->subject->getRulesAssoc($searchPattern);

self::assertSame([], $result);
}

/**
* @test
*/
public function getRulesAssocWithPatternOrdersRulesByPosition(): void
{
$first = (new Rule('font'))->setPosition(1, 42);
$second = (new Rule('font-family'))->setPosition(1, 64);
$third = (new Rule('font-weight'))->setPosition(55, 7);
$this->subject->setRules([$third, $second, $first]);

$result = $this->subject->getRules('font-');

self::assertSame([$first, $second, $third], \array_values($result));
}

/**
* @param list<string> $propertyNames
*/
Expand Down