add phpunit setup and initial tests for social api#6
add phpunit setup and initial tests for social api#6neerajp99 wants to merge 35 commits intodrupalsocialinitiative:8.x-2.xfrom
Conversation
|
There are vendor files included in the commit. Add the vendor folder to gitignore. |
|
Vendor files should never be in the module folders. Same goes for the composer.lock file. You shouldn't run |
|
Should we add assertion here to check setClient is working successfully and returning something? |
There was a problem hiding this comment.
I didn't look at everything nor added comments for everything I have reviewed. In general, you have to follow the comments I gave you and apply them everywhere. Do not only change the things I explicitly pointed out to, but also all the things that are similar to the things I pointed out. There are many things that need to be added/changed/improved. I hope you learn (and I know you will) from this and become a better coder as we go, so I don't have to add many stylistic/simple comments next time. Just remember that making something work is simple, so you have to push beyond that if you wanna be successful at this. Make it work, but also make it efficient, beautiful, easy to understand, and easy to use.
tests/src/Unit/SocialApiTest.php
Outdated
| ->getMockForAbstractClass(); | ||
| $this->assertTrue($collection instanceof UserManager); | ||
| // checking for correct setPluginId and getPluginId method | ||
| $collection->setPluginId('1234'); |
There was a problem hiding this comment.
You have to add tests for both paths of getDrupalUserId
gvso
left a comment
gvso
left a comment
There was a problem hiding this comment.
Please, read my recommendations and follow them. If you're not planning to do so, let me know, so I can stop wasting my time. I stop adding comments after the second one I added. Please, follow ALL the recommendations I have given so far and let me know when this is ready to be reviewed (I will create a label for that too).
There was a problem hiding this comment.
@neerajp99 You're not testing the real stuff. You're only testing if a method exists most of the time. That's not so useful. Think of why we want tests. We don't care much about knowing if a method exists, but what a method does.
tests/src/Unit/ControllerTest.php
Outdated
| * | ||
| * @Annotation | ||
| */ | ||
| class ControllerTest extends UnitTestCase { |
There was a problem hiding this comment.
Use the same name as in the source code, but add Test at the end (SocialApiControllerTest)
| * Tests for class SocialApiException. | ||
| */ | ||
| public function testException() { | ||
| $socialApiException = new SocialApiException(); |
There was a problem hiding this comment.
This test does not make sense. If you want to test this class, just test that the message is set correctly, etc.
|
I created a new PR for this #10 |
4 participants
No description provided.