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

Added an Error Data Listener #10477

Merged
merged 1 commit into from Jan 4, 2019

Conversation

Projects
None yet
6 participants
@mickaelandrieu
Copy link
Contributor

mickaelandrieu commented Sep 18, 2018

Questions Answers
Branch? develop
Description? Get information about silent errors in test suite
Type? bug fix
Category? TE
BC breaks? no
Deprecations? no
Fixed ticket? part of https://github.com/PrestaShop/PrestaShop/projects/1#card-13082162
How to test? Launch a test suite (composer phpunit-admin for instance)

Exemple (See travis builds)

> @php -d date.timezone=UTC ./vendor/bin/phpunit -c tests/phpunit-admin.xml
PHPUnit 5.7.27 by Sebastian Bergmann and contributors.
...............................................................  63 / 114 ( 55%)
...................................................             114 / 114 (100%)

Current report of phpErrorsHandler:
Errors: 0 / Warnings: 381 / Notices: 0 / Deprecations: 159

This change is Reviewable

namespace Tests\TestCase;
class PhpErrorsCounter

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 18, 2018

Author Contributor

ping @PierreRambaud I can throw exceptions here, but does it defeat PHPUnit configuration? :p

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 18, 2018

Contributor

We need to make some tests, with many suites, many failures, and many errors. :)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Oct 29, 2018

Author Contributor

Hmm write tests to test tests: no way xD

$this->errorsCounter = new PhpErrorsCounter();
}
public function startTestSuite(PHPUnit_Framework_TestSuite $suite)

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 18, 2018

Contributor

Could be: PHPUnit\Framework\TestSuite

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 18, 2018

Author Contributor

I tried but it breaks the interface

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 18, 2018

Contributor

:/ sad
Should we plan to update our PHPUnit version?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 18, 2018

Author Contributor

not really important right now, let's keep that for freeze period :)

@mickaelandrieu mickaelandrieu changed the title [WIP] Added Errors Data Listener Added Errors Data Listener Oct 29, 2018

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor Author

mickaelandrieu commented Oct 29, 2018

Hi @PrestaShop/prestashop-core-developers, from what I see here it's already working.

Let's improve it later if needed, but it's ready for the merge isn't it? I'm thinking about the display of each type of errors according to some environment variable.

@mickaelandrieu mickaelandrieu changed the title Added Errors Data Listener Added an Error Data Listener Oct 29, 2018

@mickaelandrieu mickaelandrieu added this to the 1.7.6.0 milestone Oct 29, 2018

@@ -6,13 +6,19 @@
colors = "true"
processIsolation = "true"
bootstrap = "bootstrap-sf.php"
convertErrorsToExceptions="true"
convertNoticesToExceptions="true"
convertWarningsToExceptions="true"
>
<php>
<server name="KERNEL_CLASS" value="AppKernel" />
<env name="SYMFONY_DEPRECATIONS_HELPER" value="weak" />

This comment has been minimized.

@jolelievre

jolelievre Oct 30, 2018

Contributor

maybe we can remove the SYMFONY_DEPRECATIONS_HELPER then?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Oct 30, 2018

Author Contributor

Not sure it's worth it, but if we do it it should be done in another pull request because it changes the behavior.

What I do here: tracking of all PHP errors,
What the Symfony deprecations helpers do: highlight every deprecation (but mostly the Symfony one).

I'm not against making PrestaShop compatible with Symfony 4, but it will create a lot of "noise" right now when our test suite has already a lot of noise for reasons we don't even understand.

This comment has been minimized.

@jolelievre

jolelievre Oct 30, 2018

Contributor

alright let's keep this for later

@jolelievre
Copy link
Contributor

jolelievre left a comment

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Oct 30, 2018

oh it seems some Codacy problems still need to be fixed

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor Author

mickaelandrieu commented Nov 5, 2018

oh it seems some Codacy problems still need to be fixed

"Avoid unused parameters such as '$suite'." => Sadly I must respect the interface

"Expected next thing to be an escaping function (see Codex for 'Data Validation'), not 'PHP_EOL'"

I'm not sure this code will be better:

hmm

wdyt?

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Nov 5, 2018

yes or maybe you could store PHP_EOL in a variable, or a local const? this would be smaller code
or store the double PHP_EOL
To be honest I don't understand why codacy doesn't like this
This leaves the issue about eh unused parameter $suite, maybe you could display the names of the suites used?

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor Author

mickaelandrieu commented Nov 6, 2018

This leaves the issue about eh unused parameter $suite, maybe you could display the names of the suites used?

You mean changing the behavior of a feature to please a static code analysis tool? Hmm I'm not really easy with it.

yes or maybe you could store PHP_EOL in a variable, or a local const?

PHP_EOL is already a constant ahahaha

I need to think a little bit about it, let's wait a little bit before merge this... maybe Codacy will be removed until then ^o^

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Nov 6, 2018

If you use a concatenation you shouldn't use printf try:

printf('%s%sCurrent report blablabla:', PHP_EOL, PHP_EOL);

If you don't use any arguments you can use print or echo

$this->suites--;
if ($this->suites === 0) {
printf(PHP_EOL . PHP_EOL . 'Current report of phpErrorsHandler:');

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 6, 2018

Contributor
printf('%s%sCurrent report of phpErrorsHandler:', PHP_EOL, PHP_EOL);
printf('%s%s%s', PHP_EOL, $this->errorsCounter->displaySummary(), PHP_EOL);
@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Nov 6, 2018

You mean changing the behavior of a feature to please a static code analysis tool? Hmm I'm not really easy with it.

yeah I see your point ^^ what if you use the suite name to track the started/finished suites instead of a counter? ^^

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Nov 30, 2018

@mickaelandrieu What's the status on this PR?

Also, I see we now have a counter, but it seems we never get details on them?

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor Author

mickaelandrieu commented Dec 2, 2018

Also, I see we now have a counter, but it seems we never get details on them?

true, it's the very first step 👍

@mickaelandrieu mickaelandrieu force-pushed the mickaelandrieu:quality/custom-error-handler branch from 935fec2 to 0b1ea32 Dec 3, 2018

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor Author

mickaelandrieu commented Dec 3, 2018

Rebased 👍

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Dec 3, 2018

I don't know if we can ask a review from the QA team on this one, adding the label just in case.

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor Author

mickaelandrieu commented Dec 10, 2018

I don't know if we can ask a review from the QA team on this one, adding the label just in case.

ping @marionf, if you can't ping one of the core team to merge. Even if it introduce a bug, it won't impact the customers.

@ntiepresta ntiepresta assigned ntiepresta and unassigned ntiepresta Dec 10, 2018

@ntiepresta ntiepresta added QA ✔️ and removed waiting for QA labels Dec 11, 2018

@Quetzacoalt91 Quetzacoalt91 merged commit 8ee6406 into PrestaShop:develop Jan 4, 2019

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
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