Skip to content
This repository has been archived by the owner on Nov 23, 2021. It is now read-only.

Core components content fragment handling fixes #391

Merged
merged 8 commits into from
Oct 22, 2019

Conversation

kaczymuczy
Copy link

@kaczymuczy kaczymuczy commented Oct 14, 2019

Description

Fixes for BB enabling the handling of Content Fragment core component
Required for: wttech/bobcat-aem-tests#13
Part of #373

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code styleguide of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

volodymyr.vashchuk added 5 commits October 10, 2019 13:59
// the application. Sometimes a warning dialog appears and it needs to be handled.
if (bobcatWait
.tweak(new TimingsBuilder().explicitTimeout(1).build())
.ignoring(Stream.of(NoSuchElementException.class).collect(Collectors.toList()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a stream?
You can use: ignoring(NoSuchElementException.class)

Copy link
Author

Choose a reason for hiding this comment

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

I was testing this solution and thought that I might add more Exceptions later. But I didn't :D

Copy link
Author

Choose a reason for hiding this comment

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

Fixed


// Branching logic in tests is not OK, it's true. But this part handles an unpredictable part of
// the application. Sometimes a warning dialog appears and it needs to be handled.
if (bobcatWait
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Try to extract the logic into a self-describing method instead of commenting.
  2. About the logic itself: are you trying to wait for the warning message -> confirm it -> verify the message disappeared?
    Few comments:
  • you are ignoring the results of the last isConditionMet - you burn some CPU time here, nothing more.
  • why the if clause in the first place? Does the message not appear every time? How about a following flow: wait for message to appear -> click it -> wait for it to disappear?

Copy link
Author

Choose a reason for hiding this comment

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

OK, will move to separate method.
The warning message doesn't always appear and I don't know yet what causes it to appear.
I know I'm ignoring the results of isConditionMet - it's a pretty sleep, nothing more, but it helps me avoid handling Selenium Exceptions

/**
* Default implementation of {@link NumberInput}
*/
@PageObject(css = Locators.NUMBERINPUT_CSS)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the locator is not used anywhere else, put it here directly.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed


bobcatWait.until(elementToBeClickable(firstResult)).click();
bobcatWait.until(elementToBeClickable(firstResult));
input.sendKeys(Keys.ENTER);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change from the previous implementation?

Copy link
Author

Choose a reason for hiding this comment

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

The previous implementation didn't support external links, like to github.com - BB would click the first result suggestem by AEM, so a page within the AEM instance instead of github.com


/**
* Default implementation of {@link Select}
*/
@PageObject(xpath = "//coral-select/..")
@PageObject(css = Locators.SELECT_CSS)
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 above - if this is the only place where the selector is used, leave it here.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

private List<WebElement> label;

@Inject
private BobcatWait wait;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not used?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed


/**
* Default implementation of {@link Textfield}
*/
@PageObject(css = Locators.FIELD_WRAPPER_CSS)
@PageObject(css = Locators.TEXTFIELD_CSS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment regarding locator

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

* Default implementation of {@link PathBrowser}
*/
@PageObject(css = Locators.AUTOCOMPLETE_CSS)
public class ContentFragmentPathBrowserImpl implements ContentFragmentPathBrowser {
Copy link
Contributor

Choose a reason for hiding this comment

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

The DefaultXYZ naming is more aligned with the rest fields

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

* #%L
* Bobcat
* %%
* Copyright (C) 2016 Cognifide Ltd.
Copy link
Contributor

Choose a reason for hiding this comment

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

Lies, it's 2019

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@mkrzyzanowski mkrzyzanowski merged commit fdad29b into master Oct 22, 2019
@mkrzyzanowski mkrzyzanowski deleted the core-components-content-fragment-handling-fixes branch October 22, 2019 14:21
@kaczymuczy kaczymuczy restored the core-components-content-fragment-handling-fixes branch October 22, 2019 14:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants