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

Behavioral tests for CMS page commands #14170

Merged
merged 20 commits into from Sep 4, 2019

Conversation

@zuk3975
Copy link
Contributor

commented Jun 12, 2019

Questions Answers
Branch? develop
Description? Adds behat tests for cms page create/edit/delete actions. ℹ️ This pr needs to wait for #14168 merge to develop for tests to pass.
Type? refacto
Category? TE
BC breaks? no
Deprecations? no
Fixed ticket? #14480
How to test? execute tests with command: ./vendor/bin/behat -c tests/Integration/Behaviour/behat.yml -s cms_page

This change is Reviewable

@zuk3975 zuk3975 requested a review from PrestaShop/prestashop-core-developers as a code owner Jun 12, 2019
/**
* @When I attempt to create cms page :reference with empty title
*/
public function attemptToCreateCmsPageWithEmptyTitle($reference)

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 13, 2019

Member

Shouldn't this be merged with createCmsPage?

throw new NoExceptionAlthoughExpectedException('Cms page creation expected to fail, but it succeeded');
} catch (\Exception $e) {
$this->latestResult = $e;

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 13, 2019

Member

Could you rename it to $lastException instead?

$cmsPageId = $this->getCommandBus()->handle($command);
SharedStorage::getStorage($reference, new \CMS((int) $cmsPageId->getValue()));
throw new NoExceptionAlthoughExpectedException('Cms page creation expected to fail, but it succeeded');

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 13, 2019

Member

I think this one is not needed, is it? 🤔

);
try {
$this->getCommandBus()->handle($command);
SharedStorage::getStorage()->set($reference, new \CMS($cmsId));

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 13, 2019

Member

If you expect exception and CMS is alkready in shared storage, what is the purpose of storing it here again?

$this->getCommandBus()->handle($command);
SharedStorage::getStorage()->set($reference, new \CMS($cmsId));
throw new NoExceptionAlthoughExpectedException('Cms page was edited, but it was expected editing to fail');

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 13, 2019

Member

You don't need to throw this exception, the next step should assert error instead.


Scenario: Attempting to create cms page for not existing category
Given cms category with id "5000" does not exist
When I attempt to create cms page "cmspage-3" with cms category id "5000"

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 13, 2019

Member

Same here, how about reusing data table? Also attempt sounds weird, how about When I create ....?

Scenario: Attempting to create cms page for not existing category
Given cms category with id "5000" does not exist
When I attempt to create cms page "cmspage-3" with cms category id "5000"
Then I should get error message '<string>'

This comment has been minimized.

Copy link
@sarjon
And cms page "cmspage-1" indexation for search engines should be disabled
And cms page "cmspage-1" should be displayed

Scenario: Attempting to edit only content field of cms page

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 13, 2019

Member

Hmmm, can't it be tested in edit scenario? Is it something specific to it?


Scenario: Attempting to add illegal script to cms page content
Given cms page "cmspage-1" "content" in default language should be "<span> content edited </span>"
When I attempt to edit cms page "cmspage-1" providing illegal content value "<form onsubmit='evil()'></form>"

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 13, 2019

Member

Same here, I don't think attempt word is needed.

And cms page "cmspage-1" "content" in default language should be "<span> content edited </span>"

Scenario: Deleting cms page
Given cms page with id "1" exists

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 13, 2019

Member

How about changing cms page to CMS Page to make it more clear? This applies for other steps as well.

@zuk3975 zuk3975 changed the title [WIP] Behavioral tests for cms page commands Behavioral tests for cms page commands Jun 19, 2019
@zuk3975 zuk3975 changed the title Behavioral tests for cms page commands Behavioural tests for cms page commands Jun 20, 2019
@matks matks added the migration label Jun 24, 2019
@zuk3975 zuk3975 changed the title Behavioural tests for cms page commands [WIP] Behavioural tests for cms page commands Jul 16, 2019
@zuk3975 zuk3975 changed the title [WIP] Behavioural tests for cms page commands [WIP] Behavioural tests for CMS page commands Jul 17, 2019
@zuk3975 zuk3975 changed the title [WIP] Behavioural tests for CMS page commands [WIP] Behavioral tests for CMS page commands Jul 17, 2019
@zuk3975 zuk3975 changed the title [WIP] Behavioral tests for CMS page commands Behavioral tests for CMS page commands Jul 18, 2019
@@ -0,0 +1,82 @@
# ./vendor/bin/behat -c tests/Integration/Behaviour/behat.yml -s cms_page
@reset-database-before-feature

This comment has been minimized.

Copy link
@tomas862

tomas862 Jul 22, 2019

Member

I think this behat test lacks multishop tests - e.g what If I create record in shop1 and shop2. You should test it as well.

This comment has been minimized.

Copy link
@matks

matks Aug 20, 2019

Contributor

Great idea, but then let's do it in a 2nd .feature file in order to ease the debugging 😉

@zuk3975 zuk3975 force-pushed the zuk3975:behat/cms branch from 1f0c5fa to 4e51725 Jul 24, 2019
@sarjon

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Tests are failing, @zuk3975 can you check?

@zuk3975

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Tests are failing, @zuk3975 can you check?

Seems that behats are not creating var/cache/purifier dir which is possibly used in CleanHtml legacy validation. Not sure if i can do something about it.

@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Tests are failing, @zuk3975 can you check?

Seems that behats are not creating var/cache/purifier dir which is possibly used in CleanHtml legacy validation. Not sure if i can do something about it.

Fixed, apparently Tools class was loading HTMLPurifier and injecting it /var/cache/dev as a cache directory because legacy was loading DEV env instead of TEST env

@@ -69,7 +69,14 @@
define('_PS_BO_ALL_THEMES_DIR_', _PS_ADMIN_DIR_.'/themes/');
}
if (!defined('_PS_CACHE_DIR_')) {
$prestashopCacheDir = _PS_ROOT_DIR_.'/var/cache/'.(_PS_MODE_DEV_ ? 'dev': 'prod'). DIRECTORY_SEPARATOR;

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Aug 20, 2019

Contributor

Currently refactoring this one in #14894

@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

 Scenario: Adding new CMS page for non existing category should not be allowed # Features/Scenario/CmsPage/cms_page_management.feature:28
    Given cms category with id "5000" does not exist                            # Tests\Integration\Behaviour\Features\Context\Domain\CmsPageFeatureContext::assertCmsCategoryWithIdDoesNotExist()
    When I create CMS page "cmspage-3" with cms category id "5000"              # Tests\Integration\Behaviour\Features\Context\Domain\CmsPageFeatureContext::createCmsPageWithProvidedCategoryId()
    Then I should get error message 'Cms page contains invalid field values'    # Tests\Integration\Behaviour\Features\Context\Domain\CmsPageFeatureContext::assertExceptionWasThrown()
      No exception was thrown in latest result (Tests\Integration\Behaviour\Features\Context\Util\NoExceptionAlthoughExpectedException)
@zuk3975

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

 Scenario: Adding new CMS page for non existing category should not be allowed # Features/Scenario/CmsPage/cms_page_management.feature:28
    Given cms category with id "5000" does not exist                            # Tests\Integration\Behaviour\Features\Context\Domain\CmsPageFeatureContext::assertCmsCategoryWithIdDoesNotExist()
    When I create CMS page "cmspage-3" with cms category id "5000"              # Tests\Integration\Behaviour\Features\Context\Domain\CmsPageFeatureContext::createCmsPageWithProvidedCategoryId()
    Then I should get error message 'Cms page contains invalid field values'    # Tests\Integration\Behaviour\Features\Context\Domain\CmsPageFeatureContext::assertExceptionWasThrown()
      No exception was thrown in latest result (Tests\Integration\Behaviour\Features\Context\Util\NoExceptionAlthoughExpectedException)

That is expected. The fix is in #14168 as stated in description.

@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

 Scenario: Adding new CMS page for non existing category should not be allowed # Features/Scenario/CmsPage/cms_page_management.feature:28
    Given cms category with id "5000" does not exist                            # Tests\Integration\Behaviour\Features\Context\Domain\CmsPageFeatureContext::assertCmsCategoryWithIdDoesNotExist()
    When I create CMS page "cmspage-3" with cms category id "5000"              # Tests\Integration\Behaviour\Features\Context\Domain\CmsPageFeatureContext::createCmsPageWithProvidedCategoryId()
    Then I should get error message 'Cms page contains invalid field values'    # Tests\Integration\Behaviour\Features\Context\Domain\CmsPageFeatureContext::assertExceptionWasThrown()
      No exception was thrown in latest result (Tests\Integration\Behaviour\Features\Context\Util\NoExceptionAlthoughExpectedException)

That is expected. The fix is in #14168 as stated in description.

Indeed, thanks ! #14168 is now waiting for QA

@zuk3975 zuk3975 force-pushed the zuk3975:behat/cms branch from 5e93840 to 7c0ef7b Aug 29, 2019
@zuk3975

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

@matks, rebased after the blocking issue merge. Seems to be working fine

@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Travis failed 🤔our behat tests did not pass

@zuk3975

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

Travis failed our behat tests did not pass

yep, again failing with that HTML purifier error. If i create new purifier dir in /var/cache/dev then it passes... not sure what should I do to fix it permanently 🤔 Maybe somewhere path needs to be changed, as i see existing purifier dir in cache/test

@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Travis failed our behat tests did not pass

yep, again failing with that HTML purifier error. If i create new purifier dir in /var/cache/dev then it passes... not sure what should I do to fix it permanently 🤔 Maybe somewhere path needs to be changed, as i see existing purifier dir in cache/test

I got it 😄when you pushed-force after rebase, you wiped out the commit I added to fix it

@zuk3975

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

Travis failed our behat tests did not pass

yep, again failing with that HTML purifier error. If i create new purifier dir in /var/cache/dev then it passes... not sure what should I do to fix it permanently thinking Maybe somewhere path needs to be changed, as i see existing purifier dir in cache/test

I got it when you pushed-force after rebase, you wiped out the commit I added to fix it

really ? I didnt see you commited something :O sorry for that, was it a big fix? (It happens second time already 😢 )
I see your message above now. My bad 😅

@matks matks dismissed their stale review via e399640 Aug 30, 2019
@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

@zuk3975 No worries 😉just re-added it using github online editor. Let's see if Travis likes it

@zuk3975

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

@zuk3975 No worries just re-added it using github online editor. Let's see if Travis likes it

okay, I instantly pulled the fresh fix and it works locally, thank you 😄

@matks

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

There is something with this PR and CI 😅 ...

@zuk3975

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

There is something with this PR and CI ...

I see, but idk what is it about 😅 . I resolved conflicts here once again anyway, and am waiting for magic 😄

@matks

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

PrettyCI is now OK but I get this crazy travis bug related to /tmp not writable

Notice: tempnam(): file created in the system's temporary directory in /home/travis/build/PrestaShop/PrestaShop/classes/PrestaShopAutoload.php on line 258
Warning: rename(/tmp/class_index.phpz6Mrg7,/home/travis/build/PrestaShop/PrestaShop/tests-legacy/PrestaShopBundle/Utils/../../../var/cache/dev/class_index.php): No such file or directory in /home/travis/build/PrestaShop/PrestaShop/classes/PrestaShopAutoload.php on line 264
Notice: tempnam(): file created in the system's temporary directory in /home/travis/build/PrestaShop/PrestaShop/classes/PrestaShopAutoload.php on line 258
Warning: rename(/tmp/class_stub.phpZARnum,/home/travis/build/PrestaShop/PrestaShop/tests-legacy/PrestaShopBundle/Utils/../../../var/cache/dev/class_stub.php): No such file or directory in /home/travis/build/PrestaShop/PrestaShop/classes/PrestaShopAutoload.php on line 264
@matks

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

This will require exploring (hence reproducing) the Travis build following PrestaShop/docs#233

@matks

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

I think this is because some part of the tests use /var/cache/dev and other parts use /var/cache/test

@matks

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

I think 41bb651 will solve the issue

@matks
matks approved these changes Sep 4, 2019
@matks

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

Thank you @zuk3975

@matks matks added this to the 1.7.7.0 milestone Sep 4, 2019
@matks matks merged commit b04c578 into PrestaShop:develop Sep 4, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.