New tests, TagArrayToStringTransformer #501

Grafikart · 2017-03-13T11:12:17Z

Added some tests to demonstrate how to mock a Repository and test things without the database

javiereguiluz

👍 if possible, please add a test condition for the usual mistake of adding too many commas (or empty tags). Example: Hello, Demo,, How and Hello, Demo, How,

Grafikart · 2017-03-13T11:23:41Z

@javiereguiluz the array_map, array_filter, explode is getting out of control, how would you improve the readability of this ? I wanted to avoid Intermediary variables :(

javiereguiluz · 2017-03-13T11:28:34Z

@Grafikart array_filter without arguments filters empty elements (I learned this nice trick here: http://stackoverflow.com/a/3654309/2804294) I don't know if it's readable ... but i's a bit shorter/simpler. Thanks!

Grafikart · 2017-03-13T11:30:12Z

Nice tip indeed

theofidry · 2017-03-13T11:58:27Z

tests/AppBundle/Form/DataTransformer/TagArrayToStringTransformerTest.php

+    }
+
+    /**
+     * Spaces at the end (and begining) of a world shouldn't matter.


beginning :)

yceruto

👍

yceruto · 2017-03-13T12:39:52Z

src/AppBundle/Form/DataTransformer/TagArrayToStringTransformer.php

@@ -54,7 +54,7 @@ public function reverseTransform($string)
            return [];
        }

-        $names = explode(',', $string);
+        $names = array_filter(array_unique(array_map('trim', explode(',', $string))));


The tagsinput plugins itself does not allows duplicated, empty tags, extra spaces, double comma (on client side), BUT it could be hacked :) so I'm 👍 to check this on server side as double validation. Maybe I would add strtolower also to keep consistency with current ones.

Not sure about strtolower, for instance I want my "JavaScript", "WordPress" tags to keep the upcases.

We could still add this transformation within the constructor

public function __construct(ObjectManager $manager, $forceStrToLower = false)

Let's keep things simple and let's not add the $forceStrToLower option.

yceruto · 2017-03-13T12:57:04Z

tests/AppBundle/Form/DataTransformer/TagArrayToStringTransformerTest.php

+     */
+    public function testUsesAlreadyDefinedTags()
+    {
+        $persisted_tags = [


Use camelCase, not underscores, for variable :) (Ref.)

oops, linter didn't caught this one ;)

Fixed with the next commit

yceruto · 2017-03-13T13:10:33Z

tests/AppBundle/Form/DataTransformer/TagArrayToStringTransformerTest.php

@@ -0,0 +1,117 @@
+<?php
+


Missing license docblock.

Fixed with the next commit

I mean app license: https://github.com/Grafikart/symfony-demo/blob/e3ab52c3063e68c420726604fd1ddf544dbbd82e/src/AppBundle/Form/DataTransformer/TagArrayToStringTransformer.php#L3-L10

yceruto · 2017-03-13T15:41:13Z

tests/AppBundle/Form/DataTransformer/TagArrayToStringTransformerTest.php

+ *
+ * See http://symfony.com/doc/current/testing/database.html
+ *
+ * @author Jonathan Boyer <[email protected]>


@author AFAIK it's not common in tests classes, instead you can move it to TagArrayToStringTransformer class ;)

Oh right misinterpreted the last review :) fixing it

I think the link should stay here to explain how mocks work. I remove the @author only right ?

That's right.

Symfony policy is to *not* add the authors in test files. Personally I don't agree ... but in this repo we must follow the Symfony practices. That's why I removed you from here ... but I added you in the PHP class associated with this test.

* Moved the helper methods at the end of the file and made them `private` * Minor changes to match the code style of the other tests

javiereguiluz

👍

javiereguiluz · 2017-03-14T11:59:42Z

It's merged now! Thanks @Grafikart for your contribution!

…reguiluz) This PR was merged into the master branch. Discussion ---------- New tests, TagArrayToStringTransformer Added some tests to demonstrate how to mock a Repository and test things without the database Commits ------- ae2391f Fixed code syntax issue reported by Travis CI 15a8d39 Minor changes 0f14c57 Added the missing license header and removed the author info 75734c6 Added Jonathan to the list of authors e3ab52c Fixed more typos b596f77 Fixed typo 72a0f85 Simplified code 3e891f5 Created a new case f69514a Merge remote-tracking branch 'origin/tag-test' into tag-test 2ca2e9b Fixed some typos 55d5bb4 Simplified some code a8de079 New tests, TagArrayToStringTransformer

Grafikart and others added 2 commits March 13, 2017 12:10

New tests, TagArrayToStringTransformer

a8de079

Simplified some code

55d5bb4

javiereguiluz reviewed Mar 13, 2017

View reviewed changes

Grafikart added 3 commits March 13, 2017 12:18

Fixed some typos

2ca2e9b

Merge remote-tracking branch 'origin/tag-test' into tag-test

f69514a

Created a new case

3e891f5

Simplified code

72a0f85

Fixed typo

b596f77

theofidry reviewed Mar 13, 2017

View reviewed changes

yceruto reviewed Mar 13, 2017

View reviewed changes

Fixed more typos

e3ab52c

yceruto reviewed Mar 13, 2017

View reviewed changes

javiereguiluz added Feature FINISHED labels Mar 14, 2017

javiereguiluz added 3 commits March 14, 2017 12:05

Added Jonathan to the list of authors

75734c6

Minor changes

15a8d39

* Moved the helper methods at the end of the file and made them `private` * Minor changes to match the code style of the other tests

javiereguiluz approved these changes Mar 14, 2017

View reviewed changes

Fixed code syntax issue reported by Travis CI

ae2391f

javiereguiluz merged commit ae2391f into symfony:master Mar 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New tests, TagArrayToStringTransformer #501

New tests, TagArrayToStringTransformer #501

Grafikart commented Mar 13, 2017

javiereguiluz left a comment

Grafikart commented Mar 13, 2017

javiereguiluz commented Mar 13, 2017 •

edited

Loading

Grafikart commented Mar 13, 2017

theofidry Mar 13, 2017

Grafikart Mar 13, 2017

yceruto left a comment

yceruto Mar 13, 2017 •

edited

Loading

Grafikart Mar 13, 2017

Grafikart Mar 13, 2017

javiereguiluz Mar 13, 2017

yceruto Mar 13, 2017 •

edited

Loading

Grafikart Mar 13, 2017

Grafikart Mar 13, 2017

yceruto Mar 13, 2017

Grafikart Mar 13, 2017

yceruto Mar 13, 2017 •

edited

Loading

yceruto Mar 13, 2017 •

edited

Loading

Grafikart Mar 13, 2017

Grafikart Mar 13, 2017

yceruto Mar 13, 2017

javiereguiluz left a comment

javiereguiluz commented Mar 14, 2017

New tests, TagArrayToStringTransformer #501

New tests, TagArrayToStringTransformer #501

Conversation

Grafikart commented Mar 13, 2017

javiereguiluz left a comment

Choose a reason for hiding this comment

Grafikart commented Mar 13, 2017

javiereguiluz commented Mar 13, 2017 • edited Loading

Grafikart commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto left a comment

Choose a reason for hiding this comment

yceruto Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

yceruto Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz left a comment

Choose a reason for hiding this comment

javiereguiluz commented Mar 14, 2017

javiereguiluz commented Mar 13, 2017 •

edited

Loading

yceruto Mar 13, 2017 •

edited

Loading

yceruto Mar 13, 2017 •

edited

Loading

yceruto Mar 13, 2017 •

edited

Loading

yceruto Mar 13, 2017 •

edited

Loading