Use 'static' instead of 'self' method call to process command result

[czukowski]

It might fix #87

[theofidry]

Could you also add the failing case you reported in the test case? I think it would be good to capture it :)

[czukowski]

It seems my test case is not principally different from one you already have here:

cpu-core-counter/tests/Finder/WmicLogicalFinderTest.php

Lines 35 to 43 in 3a46820

	yield 'example from the GitHub Actions machine' => [
	<<<'EOF'
	NumberOfLogicalProcessors
	2
	EOF
	,
	2,
	];

I think the issue is that you test it by calling the same countCpuCores which works correctly:

cpu-core-counter/tests/Finder/ProcOpenBasedFinderTestCase.php

Line 28 in 3a46820

$actual = $this->getFinder()::countCpuCores($processResult);

Whereas the real counter uses find:

cpu-core-counter/src/CpuCoreCounter.php

Line 62 in 3a46820

$cores = $finder->find();

I think ProcOpenBasedFinderTestCase should use the same route in order to capture the failure, but I'm not very familiar with your library and I suspect it would not be very easy to set up necessary mock objects since there are static class method calls involved between ProcOpenBasedFinder and its child classes. Do you have any ideas?

[czukowski]

Needless to say, I have verified that this change has fixed the issue on my machine and my test script echoed the correct number of cores 😁

[theofidry]

It seems my test case is not principally different from one you already have here:

Just for good measure maybe we can add another one without the blank line?

I think ProcOpenBasedFinderTestCase should use the same route in order to capture the failure, but I'm not very familiar with your library and I suspect it would not be very easy to set up necessary mock objects since there are static class method calls involved between ProcOpenBasedFinder and its child classes. Do you have any ideas?

Ha I see indeed. Yeah that sucks because find() is very infrastructure dependent which is why I did not test it. But this is testable elsewhere:

• create another (test) finder that extends the ProcOpenBasedFinder
• implement a universal command, e.g. echo "unparsed";
• override ::countCpuCores() core there to do something special, like return the string 'parsed'
• have a test using that finder ::find() instead of ::countCpuCores()

before your fix it would return null, after it will return 'parsed'

[czukowski]

I added a test case, although I think the extra line in between may be due to Windows line endings (CRLF), it actually doesn't appear on screen. Anyway it passed when I tried it locally and the regex looked fine to me in the first place, it would work either way.

As for testing the find method, I think you suggestion would probably work, but only incidentally, thanks to this particular command working on both Windows and Unix-based systems. Why don't you consider using dependency injection in your library? Then everything would be very easy to test in this and other situations. I understand that having this kind of change at this point is probably not very desirable, but on the other hand, I think it's fair to consider everything beyond CpuCoreCounter class your private API that you should be free to change as you see fit :)

[theofidry]

Why don't you consider using dependency injection in your library?

It's a mix of several things:

• Not wanting to over-engineer (it's already quite over-engineered IMO, by constraint but still)
• The finders were a lot less unified. They are now ProcOpenBasedFinder childs but this was not the case before
• For the proc finder one you can see injecting it to unit test it, but I also felt this was provided little benefits, and when looking at a finder like CpuInfoFinder you also see how tedious it is to test correctly

In light of this case though, I guess injecting the proc opener would be fine and at the very least it has the merit of covering close to all finders now.

I think it's fair to consider everything beyond CpuCoreCounter class your private API that you should be free to change as you see fit :)

I do plan to close the implementation details, but the finders themselves should still be part of the public API. One of the goals of the libraries over using a one liner exec() call was to provide the flexibility to enable & disable some strategies

[czukowski]

I'm not convinced you'd call it over-engineering, at least not by much :) There's already a factory in FinderRegistry, so the line with this:

cpu-core-counter/src/Finder/FinderRegistry.php

Line 50 in 3a46820

new OnlyOnWindowsFinder(new WmicLogicalFinder()),

would become something like:

new OnlyOnWindowsFinder(new WmicLogicalFinder(new ProcOpenCommandExecutor())),

As another added benefit, inheritance won't be required anymore, static method won't be required (unless you have a reason for keeping it) and the code of proc open based finders would become simpler for the first-time readers (well I guess it's a subjective measure, but I'm sure it won't be more complicated than currently). It should be even possible to make it backwards compatible if needed, but it would look a little hacky.

But anyway I guess this is outside of the scope of this issue :) For now, could you make reasonably sure this fix is actually correct and accept it? If you wish, I may send another PR with the above later when I get more time. For now I just wish to be able to use PHPStan at the full speed again :)

Oh and another thing, Github says: 'The base branch requires all commits to be signed' and I never had to bother setting this up before, could you also rebase this and use your signature?

[theofidry]

As another added benefit, inheritance won't be required anymore [...]

Inheritance will still be needed. Truth be told it could be done without, but there is some benefits to it namely that you can create a class instance for which the name reflects the command it does.

Oh and another thing, Github says: 'The base branch requires all commits to be signed' and I never had to bother setting this up before, could you also rebase this and use your signature?

Removed

[theofidry]

Thank you @czukowski

[theofidry]

Also fixed in #89