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

Refactored tests #28

Closed
wants to merge 1 commit into from

Conversation

martinssipenko
Copy link

@martinssipenko martinssipenko commented Nov 29, 2019

This PR reorganizes tests so that APCu and BlackBox tests are executed during CI flow.

Closes #26

@martinssipenko
Copy link
Author

@NoelDavies I'm also keen on moving the contents of src/Prometheus/ directory to src/ as the Prometheus is obsolete when using PSR-4. Do you reckon I should do it in this PR?

@martinssipenko
Copy link
Author

@NoelDavies any updates on this?

@NoelDavies
Copy link

@NoelDavies I'm also keen on moving the contents of src/Prometheus/ directory to src/ as the Prometheus is obsolete when using PSR-4. Do you reckon I should do it in this PR?

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.

@martinssipenko
Copy link
Author

@NoelDavies I'm also keen on moving the contents of src/Prometheus/ directory to src/ as the Prometheus is obsolete when using PSR-4. Do you reckon I should do it in this PR?

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.

Copy link

@NoelDavies NoelDavies left a 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

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?

Copy link
Author

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

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

Copy link
Author

Choose a reason for hiding this comment

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

When using delete method it internally calls doRequest with first argument as null, check it out here and here.

I guess an alternative would be to refactor delete to not use doRequest but IMHO thats not worth it given doRequest is a private method.

@martinssipenko
Copy link
Author

Any updates on this? Anything I can do to get it merged?

@NoelDavies
Copy link

@martinssipenko One of the guys at End should be able to help you out. But again, I no longer work there - Apologies.

@martinssipenko martinssipenko deleted the fix-tests branch October 30, 2023 12:07
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.

Test suite produces warnings
2 participants