[Improvement] Improve PhpUnit integration and prepare reusable constraints, resolves #340 by TheCelavi · Pull Request #350 · goaop/framework

TheCelavi · 2017-08-19T11:08:33Z

Fixes #340, created PhpUnit constraints which makes code more readable and maintainable.

Nitpick-CI · 2017-08-19T11:08:39Z

+        $validIndex = (($index = $other->getIndex()) === null);
+
+
+        foreach ($joinPoints[$access][$other->getMethod()] as $position => $expression) {


Expected 1 newline after opening brace; 2 found

Nitpick-CI · 2017-08-19T11:08:39Z

+
+        return $this->configuration['cacheDir'] . '/_proxies' . $suffix;
+    }
+}


Expected 1 newline at end of file; 0 found

Please add a newline

Added, don't worry about it, I do implement Nitpick suggestions if I miss something.

Nitpick-CI · 2017-08-19T11:08:40Z

+            return true;
+        }
+
+        foreach ($joinPoints[$access][$other->getMethod()] as $expression) {


Expected 1 newline after opening brace; 2 found

Nitpick-CI · 2017-08-19T11:08:40Z

+     */
+    public function enterNode(Node $node)
+    {
+        if (


Expected 0 spaces after opening bracket; newline found

Too long expression, one liner would make it unreadable.

For future reviews: always try to use simple checks in your if block. Extract meaningful variables and combine them with logic operator. So code will be more readable.

lisachenko · 2017-08-19T16:30:27Z

+namespace Go\PhpUnit;
+
+use \PHPUnit_Framework_Constraint as Constraint;
+use \ReflectionClass;


It isn't required to add leading slashes there, please remove them to be consistent with all codebase.

Didnt see that - IDEA added those, sorry

lisachenko · 2017-08-19T16:32:07Z

+     */
+    public function matches($other)
+    {
+        $filename = (new ReflectionClass($other))->getFileName();


What if $other isn't a class instance? Maybe add additional guard for that? To prevent uncontrolled errors

No -> actually it should fail with exception, and it should fail hard! Like, it should blow your computer up.

Constraints here are internal classes (I didn't annotated them with @internal - I didn't noticed that that you are using those) -> and they are not reusable outside AOP framework, on top of that, they should not be used outside assertThat*.

So I do not see why we should add extra guards, if it is used wrongly -> it should and it will throw exception.

ok, accepted

lisachenko · 2017-08-19T16:40:23Z

+        return
+            !file_exists($this->configuration['cacheDir'] . $suffix)
+            &&
+            !file_exists($this->configuration['cacheDir'] . '/_proxies' . $suffix);


Oh, please don't use such constructions )

We should write code for humans, not for computers, so it would be nice to do following:

add variable $transformedFileExists = file_exists($this->configuration['cacheDir'] . $suffix)

add variable $proxyFileExists = file_exists($this->configuration['cacheDir'] . '/_proxies' . $suffix)

add variable $fileHasBeenWoven = $transformedFileExists && $proxyFileExists;

just return: return !$fileHasBeenWoven;

It's simple, self-documented and readable. Also look at your expression one more time, I have a guess, that you should use || instead of && for you expression, whereas my check is valid. So it's important to give names for variables and do not inline it directly into if expressions.

If both file exists -> it is woven.
If none of file exists -> it is not woven.

If one exists and one does not -> there has been an error and assert must fail. Therefore, || should not be used.

In regards to expressions -> you have to have in mind that I am 16+ years in development and these kind of expressions are "human readable" for me.

As I said already, it is your library and I will try to follow your style and correct code, even when I disagree. But you have to cut me some slack sometimes - I do have my own style, habits and so on... these things will happen.

Sorry if my review was very personal ( When I'm looking at code and perform reviewing, I'm checking for all possible ways how to make it better and can react very emotional but it relies only to the code, no to the person who make this changes.
Of course, I feel your experience and knowledge, thank you for contributing to the framework. Excuse me if I was too rude (

lisachenko · 2017-08-19T16:40:48Z

+namespace Go\PhpUnit;
+
+use \PHPUnit_Framework_Constraint as Constraint;
+use \ReflectionClass;


Same here with leading slashes

lisachenko · 2017-08-19T16:43:04Z

+/*
+ * Go! AOP framework
+ *
+ * @copyright Copyright 2011, Lisachenko Alexander <lisachenko.it@gmail.com>


What about defining your template? ) You could use your credentials here for all new files. Also, the year will be correct. Of course, not a big deal, but why not

It is your library, I don't mind adding only your name here.

lisachenko · 2017-08-19T16:55:22Z

+        $exists     = false;
+        $validIndex = (($index = $other->getIndex()) === null);
+
+        foreach ($joinPoints[$access][$other->getMethod()] as $position => $expression) {


You could replace all this foreach code logic with following:

$advisorIndex = array_search($joinPoints[$access][$other->getMethod()], $advisorId, true); $isIndexValid = ($index === null) || ($advisorIndex === $index); return $advisorIndex !== false && $isIndexValid;

lisachenko · 2017-08-19T17:07:31Z

+
+        return $this->configuration['cacheDir'] . '/_proxies' . $suffix;
+    }
+}


Please add a newline

lisachenko · 2017-08-19T17:12:00Z

+/**
+ * Extracts join points definitions from woven class proxy by parsing and traversing class AST.
+ */
+final class JoinPointsExtractor extends NodeVisitorAbstract


Wow, looks very complicated for me.

Why not just to use ReflectionClass::getStaticPropertyValue() for asking the value of static property in the class? Of course, it will be already replaced with joinpoints, but it will be much easier than using AST for that task.

Alternatively, if you still want to parse source file, then give this job to goaop/parser-reflection, it's already on board. Just ask the same getStaticPropertyValue() to get this value for free :)

I'd vote to remove this class at all, no need in it.

Ok, here is the deal:

If it is done like I have implemented, woven class (proxy and transformed class) WILL NOT be loaded into memory, but I will manage to load join point definition in order to execute assertions. In that matter, one test case can have several tests which tests different weavings of one single class (in general, using different configurations - which is now supported). On top of that, you can load into memory original class and in same time introspect woven versions of that class.

If I use reflection class to load joint point definition, woven class will be loaded into memory, which means that one test case can test only one weaving of one single class, or for different weaving, tests must be executed in separate thread. Problem can occur especially if original class is loaded in test, and then you are trying to load same woven classes -> there will be "class is already loaded exception"

What about goaop\parser-reflection? It provides reliable API, but without loading classes into PHP memory, so you can quickly reflect any file and just return joinpoints value, do you need my help with example? And it's working on top of AST.

That was my first guess, then I introspected implementation of class Go\ParserReflection\ReflectionProperty because I needed that one.

However - not that tests are executed in one thread (PHPUnit), commands (different scope) in other. They do not share settings, they do not share class loader (Composer and AOP proxied composer).

That means that Go\ParserReflection\ReflectionEngine does not have in context of PHPUnit idea how to locate Proxy class in command scope.

That could be used if I can implement my own Locator and append it to ReflectionEngine, or, to re-init ReflectionEngine, but that will probably crash PHPUnit test context (for other tests), right?

F**** it, ReflectionEngine is singleton... tell me how to manipulate with locator and I will use your library.

To use reflections to temporarily use my locator when I need it, and then to revert back previously used one? It is hackish, but I am out of ideas....

You can easily inject own configurator with second call to the ReflectionEngine::init() and pass your own locator for test proxies or just use autoload-dev in composer for specific namespace. In this case all should work from the box. But I missed that we run code in different threads, this can make things harder.

If you have troubles with making this working, then leave it as is, it's ok.
Maybe I will have a look later to integrate existing code for loading joinpoint property value.

I can manage to work with ReflectionEngine by using reflections to access its protected static $locator property and temporarily set my own locator when I need it.

If you are OK with that -> consider it done then.

Yes, this solution is ok.
One more idea to consider: you can use ReflectionFile API to avoid even custom locator. File=>Namespace=>Class=>Static property. It doesn't require locator to be present.

lisachenko · 2017-08-19T17:15:28Z

+     */
+    public function enterNode(Node $node)
+    {
+        if (


For future reviews: always try to use simple checks in your if block. Extract meaningful variables and combine them with logic operator. So code will be more readable.

lisachenko · 2017-08-19T17:16:50Z

+
+        $ast = self::$extractor->getExtractedAst();
+
+        return eval('return ' . self::$printer->prettyPrint([$ast]));


😈 eval detected )

Eval is evil if it used without caution.

Here -> eval is used having in mind its intent and purpose and it is used properly. It is safe because source of code for eval is known and predictable. On top of that I have to be honest, I do not know how to evaluate PHP code and load it into memory (after I have extracted desired part of AST) without using eval.

If you know different method - no problemo, will implement that!

Now, you have to first make decision wether you are going to allow extraction of join points without loading woven class into memory. After that decision, we can discuss about using eval here.

BTW: https://github.com/sebastianbergmann/phpunit-mock-objects/blob/master/src/Generator.php#L263

goaop/parser-reflection can analyze and build value from AST only without using eval, you can give it a try )

Yeah, I know, I saw that implementation, a lot of work is taken there in order to avoid eval.

However - I disagree that eval is always evil, eval is evil when it is used without caution on code from unreliable source.

In this particular case, eval is not evil, it is used with care.

Problem is locator for Go\ParserReflection\ReflectionEngine -> there are two execution contexts here.

lisachenko · 2017-08-19T17:17:42Z

Today I learned about custom PhpUnit constraints 👍 Thanks!

TheCelavi · 2017-08-19T23:10:33Z

In general, I will:

Clean up and simplify code
Extend constraints to assert other weaving possibilities for class

You have to decide how do you want to load join points from woven classes, but again, have in mind that if it is not done via AST parsing, there can be conflicts in future when original class is previoysly loaded in memory during tests.

TheCelavi · 2017-08-20T09:26:01Z

WIP - don't merge it, not finished yet. However, I did some refactoring, renaming, so if you have time you could look at it at share some thoughts and suggestions, of course - and if you like me to change something - shoot!

lisachenko · 2017-08-20T14:39:59Z

+     */
+    protected function assertMethodNotWoven($class, $methodName, $advisorIdentifier, $message = '')
+    {
+        self::assertThat(new ClassAdvisorIdentifier($class, $methodName, $advisorIdentifier, AspectContainer::METHOD_PREFIX), new ClassMemberNotWovenConstraint($this->configuration), $message);


Should we care about line length? 120-140 symbols?

Agree, I can split that, no problem at all.

lisachenko · 2017-08-20T14:42:11Z

+     * @param null|int $index Index of advisor identifier, or null if order is not important.
+     * @param string $message Assertion info message.
+     */
+    protected function assertPropertyWoven($class, $propertyName, $advisorIdentifier, $index = null, $message = '')


Just quickly think about making advosorIdentifier optional. Because we can just check that there is at least key for our property. Same can be applied to methods as well. What do you think?

That is excellent idea, will do that.

lisachenko · 2017-08-20T14:43:32Z

+        $suffix   = substr($filename, strlen(PathResolver::realpath($this->configuration['appDir'])));
+
+        $transformedFileExists = file_exists($this->configuration['cacheDir'] . $suffix);
+        $proxyFileExists       = file_exists($this->configuration['cacheDir'] . '/_proxies' . $suffix);


lisachenko · 2017-08-20T14:46:52Z

+        $proxyFileExists       = file_exists($this->configuration['cacheDir'] . '/_proxies' . $suffix);
+
+        // if any of files is missing, assert has to fail
+        $classWoven = $transformedFileExists && $proxyFileExists;


lisachenko · 2017-08-20T16:28:18Z

All other stuff is 👍 ready for merge.

TheCelavi · 2017-08-20T20:13:42Z

+use ReflectionClass;
+use ReflectionProperty;
+
+final class ClassLocator implements LocatorInterface


[NEED HELP!] You can check this class, but in general, I have did my debugging, it works, locates files as I intended to be located.

TheCelavi · 2017-08-20T20:16:04Z

+    {
+        ClassLocator::initialize($this->configuration);
+
+        $advisorIdentifiers = (new ReflectionClass($class))->getStaticPropertyValue('__joinPoints')->getValue();


[NEED HELP!] @lisachenko Ok, I am stuck here. I have annotated class locator, however, I do not have any issue with that. I have tracked down issue up to:

Go\ParserReflection\Traits\ReflectionClassLikeTrait, method getStaticPropertyValue.

If I dump $properties on line no 831, I get empty array.

Do you have any idea what is going on?

I figure out what is happening. If Reflection class gets true for class_exists() at any point of time, it will not parse AST. If any of successive tests initializes original class, well, that is the end of the game...

Ok, now I am 100% sure, goaop\parser-reflection can not be used for this task. Its usage is fragile, if for any reason original class is loaded into memory, every successive test will fail. I have jumped over hoops to make it work - it is useless, sorry, I am not able deliver that in such way.

In regards to using ReflectionEngine methods for parsing classes, I do not have any benefit from it, I will get a raw AST which I will have to eventually traverse by my self manually. Tree walker gives my much more control and I can solve problem easier with less code.

In regards of using eval, since you are against it, I will try to use NodeExpressionResolver instead. If it is possible to make it work, fine, if not, eval it is, sorry.

lisachenko · 2017-08-21T09:06:54Z

@TheCelavi tests are green, changes are minimal, however need to extract logic of getting joinpoint property value into base class.

TheCelavi · 2017-08-21T09:36:33Z

Thanks!

need to extract logic of getting joinpoint property value into base class.

I am not sure, $__joinpoints property is not only thing that we need to fully assert that something is woven or not. Moreover, I didn't finished all planned asserts, there are other weavings that can be checked against, let see if this is going to be a base class, or trait, or maybe utility class....

Full picture will be known after all asserts are setup.

Thanks for help!

lisachenko · 2017-08-21T10:11:55Z

Should we squash and merge this PR? Or you want to add/polish something in it?

TheCelavi · 2017-08-21T10:15:04Z

Well, that is up to you, I have understood that you want me to work on other class weaving assertions, like properties, introductions... And I have started to work on that. So, I guess, you could wait a tiny bitsy more :)

lisachenko · 2017-08-21T10:28:27Z

Well, that is up to you, I have understood that you want me to work on other class weaving assertions, like properties, introductions

Hmmm ) We have some misunderstanding occasionally. We need to define rules, like MAY, SHOULD and MUST.

I don't want to make you doing something that wasn't planned by you earlier, we only discuss code review. So, if there was a phrase about "what if it will be property access, etc" - it was only for generalization, just additional information that MAY help you with decomposition and clean code ) That's my vision about that.

If something is really incorrect or wrong in PR, it will be described in review with direct sentence to remove/refactor/clean/etc.

I really appreciate your work and want to collaborate in effective way with you, without extra pressure from me 😄

Current PR can be merged into 2.x and you can continue your work on top of that to prevent big discussion in this PR

TheCelavi · 2017-08-21T10:37:10Z

Ok, I agree, let's split it into smaller chunks of introduced features.

I don't feel pressurized, I liked idea to have all required asserts.

Constraints are internal classes so I have locked them with final keyword. If we need to modify them later, we can do that with ease without worrying about BC breaks. If we need to extend them in the future, we can just remove final from class definition.

You can merge.

lisachenko · 2017-08-21T10:40:25Z

Squashed and merged into 2.x, thanks!

* origin/2.x: [Improvement] Improve PhpUnit integration and prepare reusable constraints, resolves #340 (#350)

lisachenko · 2017-08-21T12:30:21Z

Merged into master with applied type-hints.

Fixed goaop#340

fe90667

TheCelavi added Feature Improvement labels Aug 19, 2017

TheCelavi self-assigned this Aug 19, 2017

TheCelavi requested a review from lisachenko August 19, 2017 11:08

Nitpick-CI reviewed Aug 19, 2017

View reviewed changes

Fixed cs.

437a841

lisachenko requested changes Aug 19, 2017

View reviewed changes

lisachenko changed the title ~~Fixed #340~~ [Improvement] Improve PhpUnit integration and prepare reusable constraints, resolves #340 Aug 19, 2017

Implemented code style suggestions.

098c13b

TheCelavi added 2 commits August 20, 2017 11:21

Refactoring (renaming) and added class property weaving constraints

ba16f61

Reverted error made by IDE in refactoring. Big "UPS!"

10d2846

lisachenko reviewed Aug 20, 2017

View reviewed changes

Added tests improvements.

7187681

Issue with reflections, need help.

c6d95ca

TheCelavi commented Aug 20, 2017

View reviewed changes

Switch to parser-refelction analysis of files instead of traversing AST

1d2108d

Lock constraints as final since they are internal classes.

220cd28

lisachenko merged commit c996662 into goaop:2.x Aug 21, 2017

lisachenko added this to the 2.x milestone Aug 21, 2017

lisachenko added a commit that referenced this pull request Aug 21, 2017

Merge remote-tracking branch 'origin/2.x'

5ee0891

* origin/2.x: [Improvement] Improve PhpUnit integration and prepare reusable constraints, resolves #340 (#350)

lisachenko mentioned this pull request Aug 22, 2017

[Feature] improve base support for functional testing #353

Closed

		$validIndex = (($index = $other->getIndex()) === null);


		foreach ($joinPoints[$access][$other->getMethod()] as $position => $expression) {


		$ast = self::$extractor->getExtractedAst();

		return eval('return ' . self::$printer->prettyPrint([$ast]));

Uh oh!

Uh oh!

Conversation

TheCelavi commented Aug 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lisachenko commented Aug 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

lisachenko commented Aug 19, 2017 •

edited

Loading