2>&1 expression leads to all Psalm unrelated STDERR breaking report parsing

[Ocramius]

I'm currently working on psalm/psalm-plugin-phpunit#149

The tests there fail like this:

47) TestCase: Providers returning null are flagged
 Test  tests/acceptance/TestCase.feature:Providers returning null are flagged
Failed to parse output: 
JIT acceleration: ON

Target PHP version: 8.3 (inferred from current PHP version).

Scanning files...

579 / 579...

Analyzing files...

E
[{"link":"https:\/\/psalm.dev\/011","severity":"error","line_from":5,"line_to":5,"type":"InvalidReturnType","message":"Providers must return iterable<array-key, array<array-key, mixed>>, null provided","file_name":"509d09d0079a705e876f5aa25f1c1b063e65cf9a.php","file_path":"\/home\/ocramius\/Documents\/psalm\/phpunit-psalm-plugin\/tests\/_run\/509d09d0079a705e876f5aa25f1c1b063e65cf9a.php","snippet":"  \/** @return null *\/","selected_text":"null","from":102,"to":106,"snippet_from":88,"snippet_to":109,"column_from":15,"column_to":19,"shortcode":11,"error_level":6,"taint_trace":null,"other_references":null}]
Error:Syntax error

Scenario Steps:

 15. $I->canSeeNoErrors()
 14. $I->canSeeTheseErrors("| Type              | Message                                                                           |\n| InvalidReturnType | Providers must return iterable<array-key, array<array-key, mixed>>, null provided |")
 13. $I->runPsalm()
 12. $I->haveTheFollowingCode("class MyTestCase extends TestCase {\n  /** @return null */\n  public function provide() {\n    return null;\n  }\n  /** @dataProvider provide */\n  public function testSomething(): void {}\n}")
 11. $I->haveTheFollowingCodePreamble("<?php\nnamespace NS;\nuse PHPUnit\\Framework\\TestCase;\n")
 10. $I->haveTheFollowingConfig("<?xml version="1.0"?>\n<psalm errorLevel="1" findUnusedCode="false" %s>\n  <projectFiles>\n    <directory name="."/>\n    <ignoreFiles> <directory name="../../vendor"/> </ignoreFiles>\n  </projectFiles>\n  <plugins>\n    <pluginClass clas...")

From what I can see, the tool attempts to parse this prefix section (STDERR):

JIT acceleration: ON

Target PHP version: 8.3 (inferred from current PHP version).

Scanning files...

579 / 579...

Analyzing files...

E

I'm suggesting here that we drop the STDERR redirection in this location:

codeception-psalm-module/src/Module.php

Lines 116 to 123 in 8b8f4dd

	$cmd = $this->getPsalmPath()
	. ' --output-format=json '
	. ($suppressProgress ? ' --no-progress ' : ' ')
	. join(' ', $options) . ' '
	. ($filename ? escapeshellarg($filename) : '')
	. ' 2>&1';
	$this->debug('Running: ' . $cmd);
	$this->cli()->runShellCommand($cmd, false);

WDYT?

[weirdan]

I'd say Psalm should have a combination of flags to suppress unnecessary output. It's actually what --no-progress should do, but perhaps it's now broken in Psalm?

[weirdan]

And as for the suggestion to drop the redirection, while it'll fix the problem at hand, it'll also make debugging harder as all you'll get when Psalm crashes is empty STDOUT.

[Ocramius]

I've taken another approach (sorry, mostly did it while on the go, so I failed to respond here): Ocramius/psalm-plugin-phpunit@a9f5d78

I replaced codeception with a slimmer behat/behat, and moved all step declarations into psalm/plugin-phpunit directly, since that's where the tests are run/adjusted, and I felt like an external package only complicated maintenance (unrelated, but I had a similar issue at work, lately: https://mastodon.social/@ocramius/113962337230648729 ).

Any CLI errors are better abstracted by azjezz/psl, which (although being quite a large dependency) are exceptional in quality, and should help us tons: achieved 100% type coverage through typed parsing of STDOUT too 🚀

I'm unsure if I should keep this issue open here: lemme know your thoughts on that.

[weirdan]

Your original issue was caused by the fact that you moved the module into its own composer dependency space. This caused version checks (done via InstalledPackages) to fail and the module to assume it's running with an ancient Psalm version.

[Ocramius]

Yeah, I dropped all version checking code too, and the dependencies thereof for downstream consumers 😁

[Ocramius]

Thanks @weirdan!

[weirdan]

Before this is released I'd like to get #57 merged.