baseline: normalize newlines in error messages #5

staabm · 2019-11-06T19:56:00Z

ondrejmirtes

This isn't sufficient. You need to also make sure that a message containing \r\n is ignored by ignoreErrors item that contains only \n. And to write an integration test in https://github.com/phpstan/phpstan-src/blob/master/tests/PHPStan/Command/ErrorFormatter/BaselineNeonErrorFormatterIntegrationTest.php that verifies everything actually works together.

staabm · 2019-11-09T10:01:01Z

Addded a unit test and fed data into the integration test, which failed for us in the real world app.

hopefully that what you requested.

ondrejmirtes · 2019-11-09T12:16:38Z

We're still missing an integration test for BaselineNeonErrorFormatterIntegrationTest.php that would check that errors generated by a file with CRLF (that actually needs to be added to the repository) with baseline that generates only LF endings get also ignored.

ondrejmirtes · 2019-11-09T12:17:06Z

Because the changes in IgnoredError.php still aren't tested.

staabm · 2019-11-09T12:21:21Z

We're still missing an integration test for BaselineNeonErrorFormatterIntegrationTest.php that would check that errors generated by a file with CRLF (that actually needs to be added to the repository) with baseline that generates only LF endings get also ignored.

I got the impression that the new method I added one one of the testclasses will generate a warning including a newline and therefore the lines should be covered?
(Since this classes are used to generate a baseline and re-apply the baseline to check whether all errors get ignored)

ondrejmirtes · 2019-11-09T13:49:08Z

I think you need to add a new file that has CRLFs inside. I don't think that tests/PHPStan/Command/ErrorFormatter/data/Bar.php does.

staabm · 2019-11-10T10:05:27Z

tests/PHPStan/Command/ErrorFormatter/data/WindowsNewlines.php

moved the test into this new file, which uses windows line endings.
also added a .gitattributes file, which should make sure that the line-endings for this file are enforced to CRLF

staabm · 2019-11-10T10:24:15Z

tests/PHPStan/Command/ErrorFormatter/BaselineNeonErrorFormatterIntegrationTest.php

I guess we need to change this assertion because of the newly added testfiles under data/

ondrejmirtes · 2019-11-10T12:13:19Z

Hi, please make sure that your build passes locally running vendor/bin/phing before you push your changes. Looking at the diff, I think we're really close to merging this :)

staabm · 2019-11-10T19:20:11Z

Hi, please make sure that your build passes locally running vendor/bin/phing before you push your changes. Looking at the diff, I think we're really close to merging this :)

easier said, then done :-).

the tooling requires some non-standard php-extensions (e.g. ext-soap).
also the phing build/test-suite seem to not work on windows.. I am seeing some unrelated errors:

There were 8 failures: 1) PHPStan\Analyser\AnalyserIntegrationTest::testCollectWarnings Failed asserting that actual size 0 matches expected size 1. C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Analyser\AnalyserIntegrationTest.php:189 2) PHPStan\Analyser\AnalyserTest::testIgnoreErrorByPathAndCountMoreThanExpected Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Analyser/data/two-fails.php' +'C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Analyser\data\two-fails.php' C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Analyser\AnalyserTest.php:100 3) PHPStan\Analyser\AnalyserTest::testIgnoreErrorByPathAndCountMissing Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Analyser/data/two-fails.php' +'C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Analyser\data\two-fails.php' C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Analyser\AnalyserTest.php:144 4) PHPStan\Command\CommandHelperTest::testResolveRelativePaths with data set #0 ('C:\xampp7.3\htdocs\phpstan-sr...t.neon', array('C:\xampp7.3\htdocs\phpstan-sr...re.php', array('C:\xampp7.3\htdocs\phpstan-sr...re.php', 'C:\xampp7.3\htdocs\phpstan-sr...re.php', 'C:\xampp7.3\htdocs\phpstan-sr...up.php'), array('C:\xampp7.3\htdocs\phpstan-sr...hs/src', 'C:\xampp7.3\htdocs\phpstan-sr...-paths', 'C:\xampp7.3\htdocs\phpstan-src\conf'), array('C:\xampp7.3\htdocs\phpstan-sr...hs/src'), 'C:\xampp7.3\htdocs\phpstan-sr..._limit', array('C:\xampp7.3\htdocs\phpstan-sr...hs/src'))) Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Command/relative-paths/here.php' +'C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Command\relative-paths\here.php' C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Command\CommandHelperTest.php:230 5) PHPStan\Command\CommandHelperTest::testResolveRelativePaths with data set #1 ('C:\xampp7.3\htdocs\phpstan-sr...d.neon', array(array('C:\xampp7.3\htdocs\phpstan-sr...re.php', 'C:\xampp7.3\htdocs\phpstan-sr...re.php', 'C:\xampp7.3\htdocs\phpstan-sr...up.php'), array(array('#aaa#', 'C:\xampp7.3\htdocs\phpstan-sr...aa.php'), array('#bbb#', array('C:\xampp7.3\htdocs\phpstan-sr...aa.php', 'C:\xampp7.3\htdocs\phpstan-sr...bb.php'))))) Failed asserting that two arrays are identical. --- Expected +++ Actual @@ @@ Array &0 ( - 0 => 'C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Command/relative-paths/nested/here.php' - 1 => 'C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Command/relative-paths/nested/test/there.php' - 2 => 'C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Command/relative-paths/up.php' + 0 => 'C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Command\relative-paths\nested\here.php' + 1 => 'C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Command\relative-paths\nested\test\there.php' + 2 => 'C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Command\relative-paths\up.php' ) C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Command\CommandHelperTest.php:230 6) PHPStan\Node\FileNodeTest::testRule with data set #0 ('C:\xampp7.3\htdocs\phpstan-sr...ty.php', 'File empty.php is empty.', 1) Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'01: File empty.php is empty. +'01: File C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Node\data\empty.php is empty. ' C:\xampp7.3\htdocs\phpstan-src\src\Testing\RuleTestCase.php:136 C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Node\FileNodeTest.php:81 7) PHPStan\Node\FileNodeTest::testRule with data set #1 ('C:\xampp7.3\htdocs\phpstan-sr...re.php', 'First node in file declare.ph...clare_', 1) Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'01: First node in file declare.php is: PhpParser\Node\Stmt\Declare_ +'01: First node in file C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Node\data\declare.php is: PhpParser\Node\Stmt\Declare_ ' C:\xampp7.3\htdocs\phpstan-src\src\Testing\RuleTestCase.php:136 C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Node\FileNodeTest.php:81 8) PHPStan\Node\FileNodeTest::testRule with data set #2 ('C:\xampp7.3\htdocs\phpstan-sr...ce.php', 'First node in file namespace....space_', 3) Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'03: First node in file namespace.php is: PhpParser\Node\Stmt\Namespace_ +'03: First node in file C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Node\data\namespace.php is: PhpParser\Node\Stmt\Namespace_ ' C:\xampp7.3\htdocs\phpstan-src\src\Testing\RuleTestCase.php:136 C:\xampp7.3\htdocs\phpstan-src\tests\PHPStan\Node\FileNodeTest.php:81 FAILURES! Tests: 4370, Assertions: 9637, Failures: 8, Skipped: 96. BUILD FAILED C:\xampp7.3\htdocs\phpstan-src\build.xml:195:3: Task exited with code 1 Total time: 11 minutes 35.09 seconds

hopefully the travis build is finally green..

staabm · 2019-11-11T11:32:26Z

src/Analyser/IgnoredError.php

wdyt about moving this message-normalization into a single spot? does a class exists in which this would fit in well?

It's fine like this :)

ondrejmirtes · 2019-11-11T18:54:50Z

Hi, it's still quite weird. The formatter still generates this baseline with the \r\n:

-	message: "#^PHPDoc tag @param has invalid value \\(\\)\\: Unexpected token \"\\\\r\\\\n\\\\t \\*\", expected TOKEN_IDENTIFIER at offset 113$#"	count: 1	path: tests/PHPStan/Command/ErrorFormatter/data/WindowsNewlines.php

(I commented-out unlink in BaselineNeonErrorFormatterIntegrationTest and inspected baseline.neon in the root).

Also, if I comment out the preg_replace calls in IgnoredError.php, the tests still pass, which is weird. Can you investigate? Thanks.

ondrejmirtes · 2019-11-12T17:28:00Z

^^ My theory is that your preg_replace calls do not work at all and it's still filtering based on \r\n in both the error message and the baseline file.

ondrejmirtes · 2019-11-12T17:28:18Z

So baseline across platforms wouldn't work as expected.

…use newline-format

staabm · 2019-11-12T20:05:41Z

^^ My theory is that your preg_replace calls do not work at all and it's still filtering based on \r\n in both the error message and the baseline file.

I dont get why those chars are already escaped.. but it seems thats something that happens in phpstan somewhere... tried to fix it with the recent commits..

not 100% sure whether its correct that a newline in a neon-formatted file is represented as \\\\n

ondrejmirtes · 2019-11-12T20:11:44Z

It's a newline inside regex inside neon. But when you're replacing it, it's still clean and unescaped.

ondrejmirtes · 2019-11-12T20:13:11Z

See: https://3v4l.org/Eeuch Your code didn't work...

staabm · 2019-11-12T20:17:41Z

See: https://3v4l.org/Eeuch Your code didn't work...

I expected the string to contain real newlines and work therefore like this:
https://3v4l.org/ORRkb

but nevertheless... :-)
it seems the build is green now.

ondrejmirtes · 2019-11-12T20:33:11Z

That's not how the error messages work. I'll look into it, thanks.

staabm · 2019-11-16T12:47:58Z

@ondrejmirtes any chance to this into 0.12?
Or do you plan to check this PR after 0.12 was released?

ondrejmirtes · 2019-11-16T13:58:14Z

I plan to include it in 0.12.

ondrejmirtes · 2019-11-23T19:01:23Z

OK, I did a few more tests and realized:

There's a bug in normalizing the $ignoredErrorPattern. See the fix: 1fe9d37#diff-cbca23e35d1037391be331873c46ca8dR54
Normalizing in BaselineNeonErrorFormatter isn't necessary thanks to the fix in 1). fa4161a

Anyway, I just merged this. Thank you!

staabm · 2019-11-23T20:16:23Z

Thx for bringing it over the finish line

ondrejmirtes requested changes Nov 6, 2019

View reviewed changes

staabm force-pushed the patch-1 branch from d07480f to 1ecbc11 Compare November 9, 2019 09:29

staabm commented Nov 10, 2019

View reviewed changes

staabm mentioned this pull request Nov 10, 2019

Fix unit-test cross-OS glitches #8

Merged

staabm commented Nov 11, 2019

View reviewed changes

staabm force-pushed the patch-1 branch from 35f921f to f8a5bec Compare November 12, 2019 19:46

staabm and others added 13 commits November 12, 2019 20:54

baseline: normalize newlines in error messages

8f225e9

Update BaselineNeonErrorFormatter.php

e9099bb

Added testFormatErrorMessagesNormalizedNewlines

95c267e

fix CS

decac4f

normalize newlines to allow working with ignore-patterns indepent of …

b78c241

…use newline-format

added testcase with newlines in phpdoc

4d6204c

added docs for the docs

accec36

fix typo

fd47110

added test-file with windows line-endings

ee8fcbb

enforce windows line-endings via .gitattributes

981abea

fix CS

af2705b

fixed assertion

644a34c

fixed another assertion

bc5f53e

staabm and others added 10 commits November 12, 2019 20:54

Throw Exception on preg errors

6d5f486

Throw Exception on preg errors

dd48780

Fix CS

547d3b5

Update BaselineNeonErrorFormatter.php

33c60ef

fix CS

952e2b5

fix exception message

d7c2924

fix CS

cf9bde2

fix newline replacement

f8a5bec

fix test

582862e

added docs

fe854e0

staabm added 3 commits November 12, 2019 21:05

fix test

5a444da

fix test expectations

7e4a5b2

removed dead code

b0d008f

staabm mentioned this pull request Nov 22, 2019

Support X-OS baseline phpstan/phpstan#2558

Closed

ondrejmirtes closed this Nov 23, 2019

staabm deleted the patch-1 branch November 23, 2019 20:14

ondrejmirtes mentioned this pull request Dec 9, 2019

baseline path is OS-sensitive phpstan/phpstan#2702

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

baseline: normalize newlines in error messages #5

baseline: normalize newlines in error messages #5

Uh oh!

staabm commented Nov 6, 2019 •

edited

Loading

ondrejmirtes left a comment

staabm commented Nov 9, 2019

ondrejmirtes commented Nov 9, 2019

ondrejmirtes commented Nov 9, 2019

staabm commented Nov 9, 2019 •

edited

Loading

ondrejmirtes commented Nov 9, 2019

staabm Nov 10, 2019

staabm Nov 10, 2019

ondrejmirtes commented Nov 10, 2019

staabm commented Nov 10, 2019 •

edited

Loading

staabm Nov 11, 2019

ondrejmirtes Nov 11, 2019

ondrejmirtes commented Nov 11, 2019

ondrejmirtes commented Nov 12, 2019

ondrejmirtes commented Nov 12, 2019

staabm commented Nov 12, 2019

ondrejmirtes commented Nov 12, 2019

ondrejmirtes commented Nov 12, 2019

staabm commented Nov 12, 2019 •

edited

Loading

ondrejmirtes commented Nov 12, 2019

staabm commented Nov 16, 2019

ondrejmirtes commented Nov 16, 2019

ondrejmirtes commented Nov 23, 2019

staabm commented Nov 23, 2019

Labels

2 participants

baseline: normalize newlines in error messages #5

baseline: normalize newlines in error messages #5

Uh oh!

Conversation

staabm commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ondrejmirtes left a comment

Choose a reason for hiding this comment

staabm commented Nov 9, 2019

ondrejmirtes commented Nov 9, 2019

ondrejmirtes commented Nov 9, 2019

staabm commented Nov 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ondrejmirtes commented Nov 9, 2019

staabm Nov 10, 2019

Choose a reason for hiding this comment

staabm Nov 10, 2019

Choose a reason for hiding this comment

ondrejmirtes commented Nov 10, 2019

staabm commented Nov 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

staabm Nov 11, 2019

Choose a reason for hiding this comment

ondrejmirtes Nov 11, 2019

Choose a reason for hiding this comment

ondrejmirtes commented Nov 11, 2019

ondrejmirtes commented Nov 12, 2019

ondrejmirtes commented Nov 12, 2019

staabm commented Nov 12, 2019

ondrejmirtes commented Nov 12, 2019

ondrejmirtes commented Nov 12, 2019

staabm commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ondrejmirtes commented Nov 12, 2019

staabm commented Nov 16, 2019

ondrejmirtes commented Nov 16, 2019

ondrejmirtes commented Nov 23, 2019

staabm commented Nov 23, 2019

Labels

2 participants

staabm commented Nov 6, 2019 •

edited

Loading

staabm commented Nov 9, 2019 •

edited

Loading

staabm commented Nov 10, 2019 •

edited

Loading

staabm commented Nov 12, 2019 •

edited

Loading