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

Added a test logger #1229

Closed
wants to merge 2 commits into from
Closed

Added a test logger #1229

wants to merge 2 commits into from

Conversation

gmponos
Copy link
Contributor

@gmponos gmponos commented Nov 17, 2018

Hello,

I have created a TestLogger. I hope that you feel that this fits inside your package. I know that Monolog has a TestHandler but I believe this is better for the following reasons:

  • Simpler initialization of Logger inside tests. At the moment in order to perform my tests I need to initialize a Logger and then a TestHandler.. More properties that I have to maintain inside my code... etc etc.
  • Faster tests since it does minimal work. It skips all the extra work that it is made by Logger class.
  • Inside the TestHandler you have the getRecords function. Even if I retrieve all the records when I need to do simple asserts I can not.. Check this:
$this->assertSame(['level' => 'warning', 'message' => 'that', ['context' => []], $records[0]];

The record contains a lot of extra information that in my case they are not needed.

Even in your tests you had to do unset. So I don't need to have all these information like formatted etc etc.

All I need to test is that the API of the LoggerInterface class is called correctly.

@Seldaek
Copy link
Owner

Seldaek commented Nov 19, 2018

Could you move this inside a Monolog\Test\ namespace (but still in src/)? If you also wanna help out by taking care of #1197 at the same time that'd be even better :)

@Seldaek
Copy link
Owner

Seldaek commented Nov 19, 2018

On the other hand, I kinda wonder if this doesn't belong more in the psr/log package as it doesn't have anything monolog specific really

@Seldaek Seldaek added this to the 2.0 milestone Nov 19, 2018
@gmponos
Copy link
Contributor Author

gmponos commented Nov 20, 2018

I had the same dilemma... if I should commit it here or on the PSR. Checking PSR now again I just saw this namespace Psr\Log\Test so yes I believe it could fit in there. check the PR php-fig/log#57

Thank you.

@Seldaek
Copy link
Owner

Seldaek commented Nov 20, 2018

Ok gonna close this here then.

@Seldaek Seldaek closed this Nov 20, 2018
@gmponos
Copy link
Contributor Author

gmponos commented Nov 10, 2021

Hi @Seldaek do you think that this could be added in here now that the TestLogger is removed from psr/log ?

@stof
Copy link
Contributor

stof commented Nov 10, 2021

@gmponos as there is still nothing specific to Monolog in that TestLogger (the monolog-specific way is to use the TestHandler of Monolog), you can maintain that in its own package.

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

Successfully merging this pull request may close these issues.

3 participants