-
Notifications
You must be signed in to change notification settings - Fork 11
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
#280 configuration for selenide messages #281
#280 configuration for selenide messages #281
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.
I think it is better to set the showSelenideErrorDetails
property to true
in testErrorDetailesActived
and to false
in testErrorDetailesDeActived
and I would change the name of the tests (Actived
to Activated
).
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.
fair enough, changed it
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.
what is this? :o
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.
required to make it work, therefore, should stay
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.
what is this? :o
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.
Looks like it's really not required here. Please, remove
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.
removed
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.
cosmetic changes requested
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.
Looks like it's really not required here. Please, remove
|
||
if (Neodymium.configuration().showSelenideErrorDetails()) | ||
{ | ||
return Strings.join(super.generateErrorDetails(error, driver, screenshot, timeoutMs)); |
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.
return value is a string, so we probably don't need Strings.join here
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.
correct. removed it.
StringBuilder sb = new StringBuilder(); | ||
return sb.append("(") | ||
.append(timeout(timeoutMs)) | ||
.append(")") | ||
.toString(); |
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.
can we reuse the functionality Selenide offers?
StringBuilder sb = new StringBuilder(); | |
return sb.append("(") | |
.append(timeout(timeoutMs)) | |
.append(")") | |
.toString(); | |
return join("(" + timeout(timeoutMs) + ")"); |
or maybe with cause (because sometimes SERE may appear in cause instead of actual message and it may be useful to know that the exception was cause by SERE)
StringBuilder sb = new StringBuilder(); | |
return sb.append("(") | |
.append(timeout(timeoutMs)) | |
.append(")") | |
.toString(); | |
return join("(" + timeout(timeoutMs) + ")", causedBy(error.getCause())); |
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.
as agreed in the call, we keep the stringbuilder as is
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.
required to make it work, therefore, should stay
No description provided.