-
Notifications
You must be signed in to change notification settings - Fork 545
baseline: normalize newlines in error messages #5
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
d07480f to
1ecbc11
Compare
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.
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.
Because the changes in IgnoredError.php still aren't tested.
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)
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we need to change this assertion because of the newly added testfiles under data/
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 :)
Hi, please make sure that your build passes locally running
vendor/bin/phingbefore 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..
src/Analyser/IgnoredError.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdyt about moving this message-normalization into a single spot? does a class exists in which this would fit in well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine like this :)
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.
^^ 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.
So baseline across platforms wouldn't work as expected.
35f921f to
f8a5bec
Compare
...use newline-format
^^ My theory is that your preg_replace calls do not work at all and it's still filtering based on
\r\nin 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
It's a newline inside regex inside neon. But when you're replacing it, it's still clean and unescaped.
See: https://3v4l.org/Eeuch Your code didn't work...
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.
That's not how the error messages work. I'll look into it, thanks.
@ondrejmirtes any chance to this into 0.12?
Or do you plan to check this PR after 0.12 was released?
I plan to include it in 0.12.
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!
Thx for bringing it over the finish line
Uh oh!
There was an error while loading. Please reload this page.
Refs phpstan/phpstan#2558