Skip to content

Conversation

jfreixa
Copy link

@jfreixa jfreixa commented Oct 23, 2020

@@ -148,4 +148,9 @@ public function testGeneralize(): void
$this->assertSame('class-string', (new ConstantStringType('NonexistentClass', true))->generalize()->describe(VerbosityLevel::precise()));
}

public function testInvalidEncoding(): void
{
$this->assertIsString((new ConstantStringType(file_get_contents('invalidUtf8Characters.txt', true)))->describe(VerbosityLevel::value()));
Copy link
Author

Choose a reason for hiding this comment

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

I cannot use assertEquals or assertSame due I don't have the same string. Which assert would you use in this situation?

Copy link
Member

Choose a reason for hiding this comment

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

assertSame with the exact string produced in the test

@@ -148,4 +148,9 @@ public function testGeneralize(): void
$this->assertSame('class-string', (new ConstantStringType('NonexistentClass', true))->generalize()->describe(VerbosityLevel::precise()));
}

public function testInvalidEncoding(): void
{
$this->assertIsString((new ConstantStringType(file_get_contents('invalidUtf8Characters.txt', true)))->describe(VerbosityLevel::value()));
Copy link
Author

Choose a reason for hiding this comment

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

Is it ok to use file_get_contents in this test or I should move it to a different test?

Copy link
Member

Choose a reason for hiding this comment

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

Please produce invalid UTF-8 by using \x00 etc. characters in the source code directly.

@@ -148,4 +148,9 @@ public function testGeneralize(): void
$this->assertSame('class-string', (new ConstantStringType('NonexistentClass', true))->generalize()->describe(VerbosityLevel::precise()));
}

public function testInvalidEncoding(): void
{
$this->assertIsString((new ConstantStringType(file_get_contents('invalidUtf8Characters.txt', true)))->describe(VerbosityLevel::value()));
Copy link
Member

Choose a reason for hiding this comment

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

assertSame with the exact string produced in the test

@@ -148,4 +148,9 @@ public function testGeneralize(): void
$this->assertSame('class-string', (new ConstantStringType('NonexistentClass', true))->generalize()->describe(VerbosityLevel::precise()));
}

public function testInvalidEncoding(): void
{
$this->assertIsString((new ConstantStringType(file_get_contents('invalidUtf8Characters.txt', true)))->describe(VerbosityLevel::value()));
Copy link
Member

Choose a reason for hiding this comment

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

Please produce invalid UTF-8 by using \x00 etc. characters in the source code directly.

try {
$truncatedValue = \Nette\Utils\Strings::truncate($this->value, self::DESCRIBE_LIMIT);
} catch (\Nette\Utils\RegexpException $e) {
$truncatedValue = substr($this->value, 0, self::DESCRIBE_LIMIT);
Copy link
Member

Choose a reason for hiding this comment

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

We're also missing the ellipsis here in case the string is longer than self::DESCRIBE_LIMIT.

Copy link
Author

Choose a reason for hiding this comment

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

I added the ellipsis always because the exception is not thrown when the string is shorter than self::DESCRIBE_LIMIT.

Copy link
Member

Choose a reason for hiding this comment

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

Please add a test that short invalid UTF-8 is not truncated and ellipsis not added.

@jfreixa jfreixa requested a review from ondrejmirtes October 23, 2020 09:10
@ondrejmirtes ondrejmirtes merged commit 0dee4c3 into phpstan:master Oct 23, 2020
@ondrejmirtes
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants