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

Add status support #16

Merged
merged 11 commits into from
Jan 2, 2018
Merged

Add status support #16

merged 11 commits into from
Jan 2, 2018

Conversation

brendt
Copy link
Contributor

@brendt brendt commented Jan 2, 2018

I've noticed the Travis builds failing from time to time. To be able to debug this, I need more information about the pool. This PR adds a class which is used to print out the status of the pool as a test failing message.

An example of a failing build: https://travis-ci.org/spatie/async/jobs/323771446

Re-running builds fixes the tests, so it's not an easy to reproduce issue.

This debugger is a quick addition, but maybe can be improved upon in the future to be useful in real application too.

@brendt
Copy link
Contributor Author

brendt commented Jan 2, 2018

@freekmurze Are you ok with adding this to the codebase? It feels like a bit of a dirty fix to me, but it's the only thing I can think about to be able to debug this issue better in the future.

I noticed that xdebug is enabled on Travis, maybe there's way to replay the failing test with xdebug? That might be a better solution than adding this. Anyways, your feedback is appreciated.

{
$failed = $pool->getFailed();

$status = "\nFailed status:\n\n";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add some helpers for outputting a title, a line, ...

Copy link
Member

@freekmurze freekmurze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems nice. We could make the API a bit more developer friendly:

  • letting PoolDebugger holding an instance of Pool
  • adding a factory like method on Pooldebugger to create an instance for a Pool (maybe a constructor is enough, but I tend to use static factory methods for these kind of things)
  • adding a method on Pool to create a debugger.

So you can do things like

$pool->debugger()->status();
$pool->debugger()->summary();

or

$debugger = $pool->debugger();

$debugger->status();

Thoughts?

@brendt
Copy link
Contributor Author

brendt commented Jan 2, 2018

@freekmurze I was hesitant to tie a debugger instance to the pool, because it's not needed for the real functionality and makes the Pool class even larger, something I'd like to avoid. That's why I went with a simple helper class which can read and parse the status of the pool.

I would actually like to see some kind of PSR-compliant logging added in the future. I can see some advantages in using a logger over a static helper class. However I don't think it's a required featured for a first release, as I'd like some real user input first.

So that are my two arguments against adding an extra dependency to Pool itself. I actually believe the PoolDebugger class may be removed in the future. However, we do need a way to better debug failing builds before a 1.0 release, as it can signify an issue we're unaware of that needs fixing.

@freekmurze
Copy link
Member

I'd still add such functionality to the Pool class. Maybe we should rename it to a something that sounds more first-class-citizen-like, eg status instead of `debugger?

@brendt
Copy link
Contributor Author

brendt commented Jan 2, 2018

status indeed sounds better. Are you ok with Pool creating a new PoolStatus instance on construct, and for now only implementing PoolStatus::__toString() which we can use in the test fail messages?

@freekmurze
Copy link
Member

Sounds good to me!

@brendt brendt changed the title Add debugger Add status support Jan 2, 2018
@brendt
Copy link
Contributor Author

brendt commented Jan 2, 2018

@freekmurze Feel free to review the last changes when you've got the time :)

$this->pool = $pool;
}

public function __toString(): string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!


protected function lines(string ...$lines): string
{
return implode("\n", $lines);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could opt to use PHP_EOL here.

{
$failed = $this->pool->getFailed();

$status = '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could opt to use array_reduce in order to avoid this temporary variable.

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.

2 participants