Skip to content
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

AnnotationExtractor: fix tests on windows #111

Merged
merged 7 commits into from
May 30, 2021

Conversation

staabm
Copy link
Contributor

@staabm staabm commented May 26, 2021

fixes failing windows test

1) Rector\Tests\ChangesReporting\Annotation\AnnotationExtractorTest::testExtractAnnotationFromClass with data set "Class with changelog annotation" ('Rector\Tests\ChangesReporting...ngelog', '@changelog', 'rectorphp/...iew.md')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'rectorphp/rector@master/docs/rector_rules_overview.md'
+'rectorphp/rector@master/docs/rector_rules_overview.md
'

2) Rector\Tests\ChangesReporting\Annotation\AppliedRectorsChangelogResolver\RectorsChangelogResolverTest::test
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
-    'Rector\Tests\ChangesReporting\Annotation\AppliedRectorsChangelogResolver\Source\RectorWithChangelog' => 'rectorphp/rector@master/docs/rector_rules_overview.md'
+    'Rector\Tests\ChangesReporting\Annotation\AppliedRectorsChangelogResolver\Source\RectorWithChangelog' => 'https://github.com/rectorphp/rector/blob/master/docs/rector_rules_overview.md\r
+'
 )

@staabm staabm changed the title RectorsChangelogResolver: fix tests on windows AnnotationExtractor: fix tests on windows May 26, 2021
@staabm
Copy link
Contributor Author

staabm commented May 26, 2021

ready to land

@TomasVotruba
Copy link
Member

What exactly is going on here?

This looks like a workaround for a bug in regex. We should fix the bug in regex instead. Ideally with link to verification on https://regex101.com/

@staabm
Copy link
Contributor Author

staabm commented May 26, 2021

It seems the regex is matching \r\n and therefore the \r\n is part of the matched url at the very end

Actually it could be reduced to a rtrim. I see no easy way to support a multi line regex fixing the issue at hand.

@TomasVotruba
Copy link
Member

If rtrim() does the same work, let's go for it 👍

@staabm
Copy link
Contributor Author

staabm commented May 26, 2021

Fix verified. Ready to land

@staabm
Copy link
Contributor Author

staabm commented May 27, 2021

regarding the regex.

question is, why does https://3v4l.org/nU270 match the string without a ending newline, but https://3v4l.org/G9NNG contains the ending newline in the matched string

@staabm
Copy link
Contributor Author

staabm commented May 27, 2021

fixed the regex and reverted the rtrim workaround. ready to land.

@staabm
Copy link
Contributor Author

staabm commented May 30, 2021

@TomasVotruba good to go

@TomasVotruba
Copy link
Member

Thank you 👍 Elegant fix

@TomasVotruba TomasVotruba merged commit b42fc3d into rectorphp:main May 30, 2021
@staabm staabm deleted the changelog branch May 30, 2021 09:06
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.

3 participants