-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
Please add a link to the issue in Bobcat that you're solving so the PR appears in the comment thread for that issue |
@@ -18,5 +20,6 @@ | |||
@Override | |||
protected void configure() { | |||
bind(TextComponent.class).to(TextComponentImpl.class); | |||
bind(TeaserComponent.class).to(TeaserComponentImpl.class); |
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.
There's no need to create the TeaserComponent interface. I recommend deleting the interface and renaming the TeaserComponentImpl class to TeaserComponent. No binding in the Guice Module required then
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.
done
import com.cognifide.qa.bb.qualifier.PageObjectInterface; | ||
|
||
@PageObjectInterface | ||
public interface TeaserComponent { |
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.
See the comment for GuiceModule - no Java interface required
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.
done
import com.google.inject.Inject; | ||
import org.openqa.selenium.WebElement; | ||
import org.openqa.selenium.support.FindBy; | ||
import org.springframework.util.Assert; |
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.
Please use assertions from AssertJ or Junit only, to stay consistent throughout the project
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.
after moved assertion to test class this issue isn't exist anymore
private BobcatWait bobcatWait; | ||
|
||
public void checkDescriptionVisibility(){ | ||
Assert.isTrue(elements.isEmpty(),"Element exist when it shouldn't"); |
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.
Please keep assertions in test classes only, for maximum visibility and role separation
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.
fixed, I moved assertion into test class
…ssertion into test class
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.
Please remove all unused code and run the auto-formatter (ctrl+alt+L) on all modified files
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 suggest reducing the number of String operations on verified elements by picking different attributes to work with. For example: innerText instead of innerHtml, href instead of outerHtml, and so on
Teaser tests for wttech/bobcat#373 |
@@ -0,0 +1,17 @@ | |||
Link & Actions: |
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.
Instead of configuring multiple dialog fields in one test I suggest only to configure the verified fields, and the rest have preconfigured and created by Sling. So you then have multiple components with different configs delivered by Sling and only choose which component to work on during a test case - much faster than configuring everything with Bobcat in the UI
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.
done - I extended content on test page
…ns, extending content page, removing unnecessary code
new ResourceFileLocation("component-configs/core-components/teaser/description.yaml"))); | ||
component = page.getContent(TeaserComponent.class, 0); | ||
assertThat(component.getTeaserDescription()) | ||
.as("Check if the descriotion is configured") |
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.
description
No description provided.