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

[plugin-web-app] Record failed assertion for intercepted click #3601

Conversation

draker94
Copy link
Contributor

@draker94 draker94 commented Feb 1, 2023

No description provided.

@draker94 draker94 requested a review from a team as a code owner February 1, 2023 10:31
@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Merging #3601 (5fe1eaa) into master (321c018) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #3601      +/-   ##
============================================
- Coverage     96.98%   96.98%   -0.01%     
+ Complexity     6068     6067       -1     
============================================
  Files           855      855              
  Lines         17350    17349       -1     
  Branches       1123     1122       -1     
============================================
- Hits          16827    16826       -1     
  Misses          417      417              
  Partials        106      106              
Impacted Files Coverage Δ
...n/java/org/vividus/ui/web/action/MouseActions.java 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@draker94 draker94 force-pushed the add_assertion_for_intercepted_click branch from ddfb96d to 5fe1eaa Compare February 1, 2023 11:35
{
javascriptActions.click(element);
softAssert.recordFailedAssertion(COULD_NOT_CLICK_ERROR_MESSAGE + webDriverException);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you appending an exception? What is the resulting error?
org.vividus.softassert.SoftAssert#recordFailedAssertion(java.lang.String, java.lang.Throwable)

Copy link
Contributor Author

@draker94 draker94 Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the resulting error?

interException

Why are you appending an exception?

It has pretty enough information about the error

org.vividus.softassert.SoftAssert#recordFailedAssertion(java.lang.String, java.lang.Throwable)

I think previous way (see previous pic) in the result more readable
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO you should record the exception with stacktrace

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this case that's doubtful, stack trace won't give any useful information

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, we can expand error in Allure for stack trace
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you all agree on the matter, it is up to you how to deal with the stacktrace. However, that doesn't explain why you are appending an exception instead of its message

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why you are appending an exception instead of its message

image
🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, is it useful for the user to have the class name in error?

@valfirst valfirst merged commit a7bd4a1 into vividus-framework:master Feb 1, 2023
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.

5 participants