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

Introduced Survival tests for new modern pages #9199

Merged
merged 3 commits into from Jun 25, 2018

Conversation

Projects
None yet
4 participants
@mickaelandrieu
Contributor

mickaelandrieu commented Jun 19, 2018

Questions Answers
Branch? develop
Description? TBD
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
How to test? 👍 this add tests to the test suite so... if tests pass it's ok :)

Important guidelines


This change is Reviewable

@prestonBot prestonBot added the develop label Jun 19, 2018

$response = $this->client->getResponse();
if ($response->isServerError()) {
file_put_contents($route. '.html', $response->getContent());

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 19, 2018

Contributor

Why writing something on disk?

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 19, 2018

Contributor

If it's in case of failure, it's useless, a test shouldn't failed :D
Otherwise we need to do exactly the same thing for every tests.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 19, 2018

Contributor

Maybe we can comment this block, but doing this is the first step to be able to understand why the test is failing.

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 19, 2018

Contributor

Imagine you have a problem with your installation, you ran all tests before going to bathroom (so much coffee), you came back, and hop look like you have the same size of directory as node_modules :D
Really, it isn't a good idea to saved it in a file, every developpers have their own way to debug. Otherwise you can play with monolog verbosity and phpunit verbosity.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 19, 2018

Contributor

It can't happen to me, I never run the tests! :trollface:

I remove this block, I'll add a comment instead.

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Jun 19, 2018

$response = $this->client->getResponse();
/**

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 19, 2018

Contributor

@PierreRambaud is it better for you?

@@ -74,18 +75,23 @@ public function setUp()
$employeeMock->id_profile = 1;
$contextMock = $this->getMockBuilder(Context::class)
->setMethods(array('getTranslator', 'getBaseURL'))

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 25, 2018

Contributor

getBaseURL doesn't exists in Context class

->will($this->returnValue($this->translator));
->will(self::returnValue($this->translator));
$contextMock->method('getContext')

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 25, 2018

Contributor

I try to cover the case where Context call itself: so wrong situation but very popular in Legacy code sadly :(

@@ -688,7 +688,7 @@ public static function setCurrency($cookie)
public static function getCldr(Context $context = null, $language_code = null)
{
static $cldr_cache;
if ($context) {
if ($context && $context->language instanceof Language) {

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 25, 2018

Contributor

Having a Context doesn't ensure to have a language class here, most of the time a notice is logged but our test suite converts it to an exception.

@mickaelandrieu mickaelandrieu merged commit 90a34fc into PrestaShop:develop Jun 25, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mickaelandrieu mickaelandrieu deleted the mickaelandrieu:bogoss/sf-integration-tests branch Jun 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment