Skip to content

[TASK] Test LineName::getArrayRepresentation()#1582

Open
oliverklee wants to merge 1 commit intomainfrom
task/array-rep/line-name
Open

[TASK] Test LineName::getArrayRepresentation()#1582
oliverklee wants to merge 1 commit intomainfrom
task/array-rep/line-name

Conversation

@oliverklee
Copy link
Copy Markdown
Collaborator

The tests are basically the same as those for the parent class ValueList. The main difference is that the default separator now is a space, not a comma.

Part of #1440.

The tests are basically the same as those for the parent class
`ValueList`. The main difference is that the default separator
now is a space, not a comma.

Part of #1440.
@oliverklee oliverklee requested a review from JakeQZ May 3, 2026 09:41
@oliverklee oliverklee self-assigned this May 3, 2026
@oliverklee oliverklee added testing PRs/issues adding additional tests only, or primarily testing-focused developer-specific Issues that only affect maintainers, contributors, and people submitting PRs labels May 3, 2026
@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 73.3% (-0.02%) from 73.324% — task/array-rep/line-name into main

Copy link
Copy Markdown
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

I've never used line names (aka named grid lines).

It seems that they are user-defined identifiers (i.e. strings), and that the LineName constructor should actually only allow strings in the $components parameter, not Values (for another PR).

In the meantime, we should be consistent with what the class is representing: a non-empty list of user-defined string identifiers, which are whitespace-separated in the rendered CSS.

*/
public function getArrayRepresentationIncludesStringComponent(): void
{
$subject = new LineName(['Helvetica']);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would prefer not to have a font name, which is confusing. It should be some made-up idendifier like main-start.

Comment on lines +42 to +66
/**
* @test
*/
public function getArrayRepresentationIncludesValueComponent(): void
{
$subject = new LineName([new Size(1)]);

$result = $subject->getArrayRepresentation();

self::assertSame('Size', $result['components'][0]['class']);
}

/**
* @test
*/
public function getArrayRepresentationIncludesMultipleMixedComponents(): void
{
$subject = new LineName([new Size(1), '+', new Size(2)]);

$result = $subject->getArrayRepresentation();

self::assertSame('Size', $result['components'][0]['class']);
self::assertSame('+', $result['components'][1]['value']);
self::assertSame('Size', $result['components'][2]['class']);
}
Copy link
Copy Markdown
Collaborator

@JakeQZ JakeQZ May 3, 2026

Choose a reason for hiding this comment

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

These tests are invalid, because LineName should not accept Size in the argument list. Instead we should have a test that two string identifiers are represented (as well as that for a single string identifier which isn't a font name).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

developer-specific Issues that only affect maintainers, contributors, and people submitting PRs testing PRs/issues adding additional tests only, or primarily testing-focused

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants