-
Notifications
You must be signed in to change notification settings - Fork 57
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
EZP-28172 Behat Covering of Content Types #312
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.
Looks nice!
Few things:
- There are some small grammar issues (i should be capitalised, im -> I'm)
- It's 19 scenarios here, could we split them into smaller groups? There are actions on Content Type Groups and Content Types, so that might be a start (not sure about the mixed ones 🤔 )
src/features/ContentTypes.feature
Outdated
@@ -0,0 +1,161 @@ | |||
Feature: Content types management | |||
As a administrator |
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.
Nitpick: As an administrator
src/features/ContentTypes.feature
Outdated
Feature: Content types management | ||
As a administrator | ||
In order to customize my eZ installation | ||
I want to manage my Content types structure. |
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.
Nitpick: Content Types 😛
src/features/ContentTypes.feature
Outdated
|
||
@javascript @common | ||
Scenario: Content Type Groups context is available | ||
Given that im in context of Content Type Groups |
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.
Usually I'd use "Given that I'm on Content Type Groups page" (or something like that), I haven't seen the syntax "I'm in context of" yet - both look ok, guess we will have to decide which one we prefer/which is clearer.
BTW: I'd drop the "that" (no matter which form is chosen).
src/features/ContentTypes.feature
Outdated
@javascript @common | ||
Scenario: Changes can be discarded while creating new Content Type Group | ||
Given that im in context of Content Type Groups | ||
When i click plus button |
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'd consider changing it to "I start creating a new Content Type Group" - it would have the same code implementation, but we're expressing the intention (and not the exact action that happens).
src/features/ContentTypes.feature
Outdated
And click Discard Changes | ||
Then i shoud be redirected to Content Types context | ||
And i should see Content Type Groups list | ||
And there's nothing new on the list |
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.
That's nice!
src/features/ContentTypes.feature
Outdated
@javascript @common | ||
Scenario: Content type can be edited | ||
Given that i'm in context of Content Type update form | ||
And there's Content Type on the list |
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 shouldn't be here.
src/features/ContentTypes.feature
Outdated
Scenario: Content type can be deleted from Content Type Group | ||
Given that i'm in context of Content Type Group | ||
When i click on checkbox left to Content Type | ||
And i click on trash icon |
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.
We could combine these two steps into one: I delete Content Type X
src/features/ContentTypes.feature
Outdated
|
||
@javascript @common | ||
Scenario: Changes of Content Type are visible in Content Type Group context | ||
Given that i'm in context of just edited Content Type |
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.
Something to discuss: to what extent do we plan to allow Scenarios to rely on each other?
If I read it correctly, it depends on the previous Scenario (and maybe we could simply Edit another Content Type in this test).
src/features/ContentTypes.feature
Outdated
Given that im in context of Content Type Groups | ||
When i click plus button | ||
And fill Name field | ||
And click Discard Changes |
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 like the margins for And
, good idea
src/features/ContentTypes.feature
Outdated
Scenario: Non-empty Content type group cannot be deleted | ||
Given that i'm in context of Content Type Groups | ||
When Content Type Group is non-empty | ||
Then i can't click on checkbox left to this Content Type Group |
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'd change to Content Type Group cannot be selected
@@ -4,10 +4,13 @@ adminui: | |||
suites: | |||
adminui: | |||
paths: | |||
- '%paths.base%/vendor/ezsystems/ezplatform-admin-ui/src/features' | |||
- '%paths.base%/vendor/ezsystems/ezplatform-admin-ui/features' |
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.
Nice! 😄 We should close https://jira.ez.no/browse/EZP-28693 after it's merged, for clarity
As an administrator | ||
In order to customize my eZ installation | ||
I want to manage my Content types structure. | ||
|
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.
First two steps of every scenario can be extracted to a Background section:
Background:
Given I am logged as "admin"
And I go to "Content Types" in "Admin" tab
} | ||
|
||
/** | ||
* @When I click :button in right menu |
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.
Isn't it already in Content Context? I click on the edit action bar button "Publish"
*/ | ||
public function iFillField(string $fieldName, string $value) | ||
{ | ||
if ($this->utilityContext->getElementByText(sprintf(/*"\"%s\"",*/ $fieldName), '.form-control-label') !== null) { |
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 is "I set field to X" in Content Context, can't it be used? +ContentContext looks like a better place for this Step
- IMHO we can change getElementByText to throw an exception in case it should return
null
, we could avoid writing this if/else every time. - sprintf can be omitted
$this->context->getElementByText($name, $this->fields['listElementLink'])->click(); | ||
} | ||
|
||
public function checkListElement(string $name): bool |
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.
It returns bool
, I'd consider renaming it to something indicating that (isElementPresent
/isElementOnList
?)
|
||
private function assertProperList(): void | ||
{ | ||
if (strcmp($this->listHeader, $this->context->findElement($this->fields['listHeader'], 5)->getText()) !== 0) { |
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.
We might: use ===
for comparison and replace 5 with $this->defaultTimeout (which is also equal 5)
|
||
class Breadcrumb extends Element | ||
{ | ||
private $fields = ['breadcrumbItem' => '.breadcrumb-item', |
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 like storing the selectors in an array 👍
class ElementFactory | ||
{ | ||
/** | ||
* Creates a Page object based on given Page Name. |
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 know you know this, just so that it does not get lost: remember about amending PHPDocs 😛
/** | ||
* @var \EzSystems\EzPlatformAdminUi\Behat\PageElement\UpperMenu | ||
*/ | ||
public $upperMenu; |
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.
Maybe we can move it to Page?
|
||
public function verifyVisibility(): void | ||
{ | ||
$this->context->checkVisibilityByClass($this->fields['plusButton']); |
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.
It won't work: checkVisibility returns bool (does not throw).
We can use:
Assert:assertTrue(checkVisibilityByClass());
or (assuming element exists):
Assert:assertTrue(findElement(selector)->isVisible());
I guess the name is misleading, we might want to change that.
You must re-base here. |
$msg = ' non'; | ||
} | ||
|
||
if(($contentsCount !== '0') === $empty){ |
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'd rename the param to shouldBeEmpty
(and extract $contentsCount !== '0'
to a new variable, isEmpty).
features/ContentTypeGroup.feature
Outdated
Given I am logged as "admin" | ||
And I go to "Content Types" in "Admin" tab | ||
|
||
@javascript @common | ||
Scenario: Content Type Groups context is available | ||
Then I should see "Content Type Groups" list |
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'm wondering if we need this scenario because it is implicitly checked by the other more advanced ones anyway.
features/ContentTypeGroup.feature
Outdated
Then I should see "Content Type Groups" list | ||
|
||
@javascript @common | ||
Scenario: Changes can be discarded while creating new Content Type Group | ||
Given I am logged as "admin" | ||
And I go to "Content Types" in "Admin" tab | ||
When I start creating new "Content Type Group" |
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.
Scenario starting from When doesn't look right - what do you think guys? (Given only in background)
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.
Both version would look fine to me (both are readable).
* | ||
* @return LoginPage|DashboardPage|ContentStructurePage|ContentTypeGroupsPage|UpdateItemPage to interact with | ||
*/ | ||
public static function createPage(UtilityContext $context, string $pageName): Page | ||
public static function createPage(UtilityContext $context, string $pageName, string $parameter = null): Page |
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.
Maybe it should be a dictionary? I can see us adding "parameter2" soon 😛
|
||
public function verifySuccessMessage(string $itemType, string $itemName, string $action): void | ||
{ | ||
printf($this->context->findElement($this->fields['successAlert'])->getText()); |
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.
* User: maciejtyrala | ||
* Date: 26/01/2018 | ||
* Time: 11:36 | ||
*/ |
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 change (Storm can be configured to add proper headers automatically).
|
||
public function verifyVisibility(): void | ||
{ | ||
$this->context->waitUntilElementIsVisible($this->fields['successAlert'], $this->defaultTimeout); |
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 don't think it should be based on successAlert (we can expect various alerts).
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.
You're right, I'll fix it. I was assume that there will be potentially more success alerts than the failure ones.
Yet, it's not something critical, because this element isn't assigned to any page, just like upperMenu, so verifyVisibility method will be call only on demand.
public function verifySuccessMessage(string $itemType, string $itemName, string $action): void | ||
{ | ||
printf($this->context->findElement($this->fields['successAlert'])->getText()); | ||
$this->context->assertElementContainsText($this->fields['successAlert'], sprintf("%s '%s' %s.",$itemType, $itemName, $action)); |
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.
Should it happen here? I'd place the assertion in NotificationContext (and simply change this one to "getText").
src/lib/Behat/PageElement/Dialog.php
Outdated
protected $fields = ['yesButton' => '', | ||
'noButton' => '', ]; | ||
protected $fields = ['confirm' => '.btn--trigger', | ||
'decline' => '.btn--no', ]; | ||
/** @var string Name by which Page is recognised */ |
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.
Page => Element 😛
features/ContentTypeGroup.feature
Outdated
Then I should see "Content Type Groups" list | ||
|
||
@javascript @common | ||
Scenario: Changes can be discarded while creating new Content Type Group | ||
Given I am logged as "admin" | ||
And I go to "Content Types" in "Admin" tab | ||
When I start creating new "Content Type Group" |
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.
Both version would look fine to me (both are readable).
{ | ||
return $this->context->getElementByText($name, $this->fields['listElementLink']) !== null; | ||
} | ||
|
||
public function verifyVisibility(): void | ||
public function checkListItemAttribute(string $name, string $header): string |
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.
This one returns texts (it does not check anything) - can't it be getTableCell($rowName, $columnName) / (getAdminTableCellValue)?.
To be honest, I don't like methods with "check".
We have:
checkListItemAttribute
which does not check anything
checkContentsStatus
which throws
checkListElementExists
, checkListElementSelectable
which do not throw and return bool
Maybe we could follow the following convention:
- if it throws: use
assert
/verify
(verifyAssignedContentCount
?) in the name? - if it returns bool: sort-of-a-question:
isElementOnTheList
,isElementSelectable
What do you think?
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.
You're right, it might be confusing and it's a bad inconsistency.
I'll fix it. Convention that you mention looks good for me.
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 nice!
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.
👍
@lserwatka ready for merge |
@webhdx @Nattfarinn @alongosz @mikadamczyk @ViniTou Could you Guys have a look on PHP code? |
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, fix SC and Type Hinting
use EzSystems\StudioUIBundle\Tests\Behat\PageObject\LandingPageEditorPage; | ||
use EzSystems\EzPlatformAdminUi\Behat\Helper\UtilityContext; | ||
|
||
//use EzSystems\StudioUIBundle\Tests\Behat\PageObject\LandingPageEditorPage; |
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.
Comment to remove
|
||
class AdministrationContext extends BusinessContext | ||
{ | ||
private $itemCreateMapping = ['Content Type Group' => ContentTypeGroupsPage::PAGE_NAME, |
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 we should follow code style also in tests
private $itemCreateMapping = [
'Content Type Group' => ContentTypeGroupsPage::PAGE_NAME,
'Content Type' => '',
....
'User' => '',
];
'Role' => '', | ||
'Section' => '', | ||
'User' => '', ]; | ||
private $emptyHeaderMapping = ['Content Type Groups' => 'Content Types count', |
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.
Same CS here
* @Then there's :listElementName on :page list | ||
* @Then there's :listElementName on :parameter :page list | ||
*/ | ||
public function isElementOnTheList($listElementName, $page): void |
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.
Missing Type Hinting?
* @Then there's no :listElementName on :page list | ||
* @Then there's no :listElementName on :parameter :page list | ||
*/ | ||
public function isElementNotOnTheList($listElementName, $page, $parameter = null): void |
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.
Type Hinting?
*/ | ||
public function isElementNotOnTheList($listElementName, $page, $parameter = null): void | ||
{ | ||
$actual = PageObjectFactory::createPage($this->utilityContext, $page, $parameter) |
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.
Better variable name?
*/ | ||
private function verifyContentsStatus($itemName, $page, $shouldBeEmpty): void | ||
{ | ||
$isEmpty = '0'; |
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.
Better variable name?
*/ | ||
public function itemCannotBeSelected($itemType, $itemName): void | ||
{ | ||
$checkboxState = PageObjectFactory::createPage($this->utilityContext, $this->itemCreateMapping[$itemType]) |
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.
Better variable name?
|
||
private function verifyProperList(): void | ||
{ | ||
if ($this->listHeader !== $this->context->findElement($this->fields['listHeader'], $this->defaultTimeout)->getText()) { |
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 extract $this->context->findElement($this->fields['listHeader'], $this->defaultTimeout)->getText()
to method or variable to improve readability?
Fixed |
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.
Some nitpicks 🙃
*/ | ||
public function iStartCreatingNew(string $newItemType): void | ||
{ | ||
if (!\array_key_exists($newItemType, $this->itemCreateMapping)) { |
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 don't prefix functions with \
. In case of built-in classes, it's fine but I rather try to add use
statement for them as well.
public function iStartCreatingNew(string $newItemType): void | ||
{ | ||
if (!\array_key_exists($newItemType, $this->itemCreateMapping)) { | ||
throw new \InvalidArgumentException(sprintf('Unrecognised item type name: %s', $newItemType)); |
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.
Typo: unrecognized
if (!$isElementOnTheList) { | ||
throw new ElementNotFoundException( | ||
$this->utilityContext->getSession(), | ||
sprintf('Element "%s" is not on the %s list.', $listElementName, $page)); |
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.
CS: please move );
to the new line, on the same level as throw
(I'm surprised cs-fixer didn't complain ;))
if ($isElementOnTheList) { | ||
throw new ElementNotFoundException( | ||
$this->utilityContext->getSession(), | ||
sprintf('Element "%s" is on the %s list.', $listElementName, $page)); |
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.
Same comment as above.
* | ||
* @param $itemName | ||
* @param $page | ||
* @param $shouldBeEmpty |
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.
Missing parameter types in PhpDoc (still required despite placing them in prototype).
case Notification::ELEMENT_NAME: | ||
return new Notification($context); | ||
default: | ||
throw new \InvalidArgumentException(sprintf('Unrecognised element name: %s', $elementName)); |
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.
Typo: Unrecognized
*/ | ||
namespace EzSystems\EzPlatformAdminUi\Behat\PageElement; | ||
|
||
class Notification extends Element |
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 didn't pay attention in similar cases, but it's a good example of needed PhpDoc, stating something like "Element describing system notifications for the user". There is another feature with that name related to EE which gets us confused very often ;)
|
||
public function verifyVisibility(): void | ||
{ | ||
// TODO: Implement verifyVisibility() method. |
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.
Same comment as previously - is it postponed for later on?
return new UpdateForm($context); | ||
case Breadcrumb::ELEMENT_NAME: | ||
return new Breadcrumb($context); | ||
case Notification::ELEMENT_NAME: |
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.
This is not very SOLID, I'm also not a huge fan of static factory methods. If any new element appears, this list needs to be updated and maintained. The problem here is that we're a little bit limited due to lack of DI container.
if I were to change it - since the switch cases are really simple - I'd just avoid using factory in favor of creating of objects in-place. It actually would produce less code :-)
POV @Nattfarinn @webhdx
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.
Actually i had the same opinion, but when i start working on automated tests, there was already PageObjectFactory, and after discussion we decided to stay with this concept. I'll be happy to discuss it again, but if it's possible, as a new task.
case UpdateItemPage::PAGE_NAME: | ||
return new UpdateItemPage($context); | ||
case ContentTypeGroupPage::PAGE_NAME: | ||
return new ContentTypeGroupPage($context, $parameters[0]); |
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.
And I see now how the ElementFactory
appeared, since here we have already existing similar code.
Still not a huge fan, but let's discuss it internally maybe.
Except of "TODO" and "Factory" (which i explained above) I think I fixed all pointed problems. I hope this time everything is ok. |
Ok, that works for me 👍 |
Feature file with scenarios for Content Types testing
Checklist:
$ composer fix-cs
)