-
-
Notifications
You must be signed in to change notification settings - Fork 717
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
Make Owner addressable via RFC 2822 standard #2411
base: main
Are you sure you want to change the base?
Conversation
bf053b0
to
00bae2e
Compare
00bae2e
to
3379a0c
Compare
@baev, I had time this morning to work on your suggested changes, and I hope the PR looks good enough now. I verified my change manually end-to-end on localhost: |
3379a0c
to
4c28665
Compare
@baev hi, just making sure you're aware that I have fixed the code according to your review. |
thx, I'm aware. Review is WIP, will try to finish it this week |
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.
Code LGTM.
Added files:
allure-generator/src/main/java/io/qameta/allure/owner/OwnerAddress.java
allure-generator/src/main/java/io/qameta/allure/owner/OwnerAddressParser.java
allure-generator/src/test/java/io/qameta/allure/owner/OwnerAddressParserTest.java
Modified files:
allure-generator/src/main/java/io/qameta/allure/owner/OwnerPlugin.java
allure-generator/src/main/javascript/plugins/testresult-owner/OwnerView.hbs
void shouldReturnNullForEmptyInput() { | ||
assertNull(OwnerAddressParser.parseAddress("")); | ||
} | ||
|
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.
Tests look good.
For better test coverage it would be good to add a test for invalid URL and invalid email.
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.
@A1exKH, I added
implementation("commons-validator:commons-validator:1.7")
to Allure Generator build.kts
to make it more resilient when it comes to validating URLs and emails. The remaining regexp for angle brackets parsing has been made a bit stricter too.
Please review the updated tests and implementation again. 🙏
4c28665
to
4e999be
Compare
@@ -32,20 +32,28 @@ void shouldReturnNullForEmptyInput() { | |||
|
|||
@Test | |||
void shouldParseRFC2822FormattedStringWithEmail() { | |||
String input = "John Doe <[email protected]>"; | |||
String input = "John Doe < [email protected] >"; |
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.
Empty spaces in the angle brackets are considered normal thing by the standard.
4e999be
to
caee19a
Compare
} | ||
|
||
// Prevent performance degradation for plain text | ||
if (!isLikelyAddress(maybeAddress)) { |
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 thing is important to have for plain text. On 10,000 "John Doe {{counter}}" addresses, it eats them in 20ms, just like without parsing (16-18ms).
Without this optimization, it takes up to 200ms on my machine to parse 10,000 addresses.
I measured the performance between my previous simplistic regexp implementation and the actual implementation with Apache validators, and the impact was negligible — ±20-30ms (sometimes faster, sometimes slower), so I hope this is totally okay for you to accept this PR with industry-proven validators for emails and URLs instead of improvised ones by me.
caee19a
to
8b73a17
Compare
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.
Code LGTM.
Context
I find it inconvenient to have
owner
just as a non-actionable label – especially in environments where a person has different names in real life, email, Github and Slack/Discord.Therefore, I'd like to suggest improving the rendering and making it follow Internet Message Format, e.g.:
and also be able to consume plain e-mail and URL addresses:
In the case when the owner is not a valid URL or e-mail, the rendering stays the same, e.g.:
Checklist