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

Added tester for RouterLink. #1810

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

joelpop
Copy link
Collaborator

@joelpop joelpop commented Jul 8, 2024

Description

Adds missing ComponentTester for RouterLink. (Attempting to use AnchorTester in its place results in an error due to Anchor and RouterLink being different Components.

Tester methods include

  • getHref
  • getPath
  • getQueryParameters
  • getRoute
  • click

Fixes #1805

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Additional for Feature type of change

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

@joelpop joelpop self-assigned this Jul 8, 2024
@joelpop joelpop added the UITest JUnit testing the UI label Jul 8, 2024
Comment on lines +46 to +48
var targetlessRouterLink = $routerLinkView.find(RouterLink.class)
.withText("No Target")
.single();
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest making all RouterLink components as package protected class fields so that the test can directly wrap the instance, without relying on find.
This allows to prevent false positives in tests because of potential bugs in the component locator.
IMO it also simplifies the test code

Copy link
Contributor

Choose a reason for hiding this comment

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

I would revert changes to this file that are not related to RouterLink

*/
public Component click() {
if (getRoute().isEmpty()) {
throw new IllegalStateException();
Copy link
Contributor

Choose a reason for hiding this comment

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

This exception requires an explanatory message

*
* @return navigated view
*/
public Component click() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about returning HasElement to be consistent with AnchorTester?

Copy link
Contributor

@mcollovati mcollovati Aug 5, 2024

Choose a reason for hiding this comment

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

The method should ensure that the component is usable by calling ensureComponentIsUsable().
I mean, the check is done indirectly done by getPath(), but I think it would be good to make it more explicit.

Comment on lines +81 to +83
Assertions.assertDoesNotThrow(() -> $targetView.find(Span.class)
.withText("Static Target View")
.single());
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as for RouterLink, what about having the Span as a package protected field and simply assert on its getText()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UITest JUnit testing the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add missing RouterLinkTester for RouterLink
2 participants