-
Notifications
You must be signed in to change notification settings - Fork 80
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
Refactored tests #28
Refactored tests #28
Conversation
@NoelDavies I'm also keen on moving the contents of |
@NoelDavies any updates on this? |
I'd probably create a new PR for this and add it to a major release as people maybe including it directly. or have custom autoloaders, etc. |
In that case this PR is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed a few things, let me know what you think.
RUN pecl install redis-2.2.8 && docker-php-ext-enable redis | ||
RUN pecl install apcu-4.0.11 && docker-php-ext-enable apcu | ||
RUN pecl install redis && docker-php-ext-enable redis | ||
RUN pecl install apcu && docker-php-ext-enable apcu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these be versioned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any reason why they should be, I think using latest should be fine for testing, or you think it matters?
* @param string $job | ||
* @param array $groupingKey | ||
* @param string $method | ||
* @throws GuzzleException | ||
*/ | ||
private function doRequest(CollectorRegistry $collectorRegistry, string $job, array $groupingKey, $method): void | ||
private function doRequest(?CollectorRegistry $collectorRegistry, string $job, array $groupingKey, $method): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First argument shouldn't be nullable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any updates on this? Anything I can do to get it merged? |
@martinssipenko One of the guys at End should be able to help you out. But again, I no longer work there - Apologies. |
This PR reorganizes tests so that APCu and BlackBox tests are executed during CI flow.
Closes #26