Skip to content
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

[Todo] Write Tests #5

Closed
irazasyed opened this issue Jul 3, 2015 · 11 comments
Closed

[Todo] Write Tests #5

irazasyed opened this issue Jul 3, 2015 · 11 comments

Comments

@irazasyed
Copy link
Owner

Need to write tests.

@jonnywilliamson
Copy link
Contributor

Ugh. Don't make me learn this stuff! My brain can't keep up!

@irazasyed
Copy link
Owner Author

It's good to learn though..lol For long run it'll be helpful to automatically test things :D

@antoniomadonna
Copy link

Hi, I've pushed some tests, covering the basics. Hope it helps!

@antoniomadonna
Copy link

Do composer install to download PHPUnit and Prophecy
Run tests with vendor/bin/phpunit --bootstrap vendor/autoload.php tests

@zhukovra
Copy link

@antoniomadonna, providing configuration for Phpunit via XML (bootstrap and other variables) is better way: https://phpunit.de/manual/current/en/organizing-tests.html#organizing-tests.xml-configuration

@antoniomadonna
Copy link

ok @zhukovra added config file

irazasyed added a commit that referenced this issue Nov 25, 2015
@jonnywilliamson
Copy link
Contributor

@antoniomadonna @irazasyed

Excellent. This is a good start. I have the tests installed and all passing here.

I do have a few suggestions. I've only just started to implement tests in my own projects, so it's a learning curve for me but here's some thoughts.

1) Composer.lock

We now need to ensure that a composer.lock is now committed to this repository. It's a very bad idea to tell us all to pull down the new changes to the composer.json file and do a composer update / composer install without having a lock file in the repository.

Without the lock file, we can NOT be certain we are all pulling in exactly the same version of the dependencies. (In fact you will receive a Warning: The lock file is not up to date with the latest changes in composer.json warning if you do this.

@irazasyed - You need to commit a composer.lock file that WE all download. Then we will be using the exact dependencies that you are using. Hence we will all be on a level playing field.

2) Guzzle Stereo

I've started to use https://github.com/ikwattro/guzzle-stereo on some of my projects to capture and playback actual API requests from a real life API.

This allows us to hit the API with some requests, save them to a json file, and then use them over and over again to test other parts of the code without having to hit the real Telegram API servers. It works really well.

Perhaps we could integrate some of these into some functional tests at a later date.

3) How the tests are written.

If you have a few minutes, please take a look at this video from jeffrey_way at laracasts.
https://laracasts.com/series/testing-jargon/episodes/1

It's a wonderful lesson/suggestion on how we should be naming our tests and WHY. In this short video I think he's using phpspec and not phpunit, but it doesn't matter...it's not the testing framework he's using that's important but the way he describes what functionality each test should achieve.

Have done this now for a few weeks, I really see the benefit of doing this. It kinda frees up your mind a little.

It's very easy to do in phpunit too. All we do is change this

    public function testAddsMultipleCommands()
    {
        $this->api->addCommands([MockCommand::class, MockCommandTwo::class]);
        $commands = $this->api->getCommands();
        $this->assertInstanceOf(MockCommand::class, $commands['mycommand']);
        $this->assertInstanceOf(MockCommandTwo::class, $commands['mycommand2']);
    }

To this:

/** @test */
public function it_checks_the_correct_number_and_type_of_commands_get_added()
{
        $this->api->addCommands([MockCommand::class, MockCommandTwo::class]);
        $commands = $this->api->getCommands();
        $this->assertInstanceOf(MockCommand::class, $commands['mycommand']);
        $this->assertInstanceOf(MockCommandTwo::class, $commands['mycommand2']);
        $this->assertCount(2, $this->api->getCommands());    
}

I'm interested to hear your feedback on this.

@irazasyed
Copy link
Owner Author

@jonnywilliamson Thanks for writing your suggestions. They really look good.

Here's my response. I'm going one by one, same as your points.

  1. composer.lock - Well, As long as it won't conflict with devs dependencies, then i can commit this file. By conflict, i mean if they're also requesting for one of our dependencies in their project with some other version?
  2. That looks interesting, Will check it out and try implementing when i start writing the tests. Right now playing with travis & phpunit configs.
  3. I had seen that video last year, But never actually implemented it. Since this was a start, Will definitely apply all these along with other best practises.

Thanks!

@antoniomadonna
Copy link

@jonnywilliamson thanks for your feedback :)

  1. as @irazasyed wrote above, I think that pushing one of our composer.lock would be the same problem as requiring devs to do composer update, because each one could already have different versions. I think the sense of providing version constraints in composer.json is to be able to safely do composer update, without breaking things. Correct me if I'm wrong of course!
  2. Nice project :) I'm going to have a look at it. At the moment the solution we're using is similar, and is based on some Guzzle testing features, so we're not hitting the real API but providing a fake response object. It could be useful to implement more realistic responses for integration tests.
  3. Naming is really not my best skill, so suggestions like this are welcome!

@jonnywilliamson
Copy link
Contributor

It could be useful to implement more realistic responses for integration tests.

Exactly, I think your method in the unit tests is very good. It just might be more useful in integration tests later.

The composer.lock issue caused problems last night. I'm attempting to get more information on from the composer author.

@jonnywilliamson
Copy link
Contributor

Hi,

So I spent the morning trying to see if I can expand on the tests that have been provided so far. I think I've made a reasonable attempt at it.

#76

I decided I wanted to extract all the mocking of telegram api responses and update responses into their own class. I didn't like having that all in one test file.

I made the TGMock file with static methods as I think this shouldn't cause any problems and it makes it pretty straightforward when you want to grab a mocked object. If people think this it is better to DI it into ApiTest.php and use it normally then I'm sure that's fine.

All tests were passing when I committed. I'm interested to see what you think. There's no point me continuing if I'm making mistakes.

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

No branches or pull requests

4 participants