Turn TraitsCachingIssueIntegrationTest into a e2e test

[staabm]

quoting thg2k:

Due to the recent addition of the random execution order in phpunit.xml, the test TraitsCachingIssueIntegrationTest::testCachingIssue was failing due to the expectation to be executed exactly in the data order.

turned into e2e test as requested in #3142 (comment)

[staabm]

//cc @thg2k

[phpstan-bot]

This pull request has been marked as ready for review.

[thg2k]

Sorry, I am a bit ignorant regarding github CI... What does this PR do actually besides adding more complexity to the system? Now there is yet one more line in that obnoxiously long list of checks...

I mean, I understand it should be moved away from unit tests, as this more e2e since it invokes phpstan itself, but why does it have to be in its own check?

[staabm]

e2e tests use separate jobs per design in the test-suite.

Ondrej mentioned he had other problems with the test case recently, but I don't know which.

The test itself is pretty slow and does not need to be run locally on every change when running make IMO, so that could be a reason to only run it in CI.

[staabm]

I guess ondrej wants to get rid of phpunit for this test-cases but lets see

[ondrejmirtes]

You misunderstood. I don't want the e2e-tests.yml to run PHPUnit. I want it to do the same steps the test case currently does but in Bash.

[thg2k]

You misunderstood. I don't want the e2e-tests.yml to run PHPUnit. I want it to do the same steps the test case currently does but in Bash.

@ondrejmirtes I have two concerns with this approach, I'd like to hear your wisdom on both:

1. They are entirely scripted inside the github workflow, so you can't really test it in command line before pushing. Ideally each one should be an indipendent bash script usable from the working copy, and the github workflow should just invoke the script file.
2. Each one creates a different "check", which concurs to this gigantic list to scroll. With the last commit they are 4 more even. Wouldn't it be better if they were all invoked one after the other in a single check? Potentially without stopping if one fails, like with the unit tests in phpunit. Is there some magic github action strategy to do this?

I think the different "checks" in github should more be like "PHP 7.x e2e" and "PHP 8.x e2e", rather than "test this" "test that"..

[thg2k]

Doesn't this second clear-result-cache defeat the purpose of the test?

[staabm]

the phpunit tests worked the same way, see

phpstan-src/tests/PHPStan/Analyser/traitsCachingIssue/TraitsCachingIssueIntegrationTest.php

Lines 117 to 140 in e45ba45

	private function runPhpStan(): array
	{
	$phpstanBinPath = __DIR__ . '/../../../../bin/phpstan';
	exec(sprintf('%s %s clear-result-cache --configuration %s', escapeshellarg(PHP_BINARY), $phpstanBinPath, escapeshellarg(__DIR__ . '/phpstan.neon')), $clearResultCacheOutputLines, $clearResultCacheExitCode);
	if ($clearResultCacheExitCode !== 0) {
	throw new \PHPStan\ShouldNotHappenException('Could not clear result cache.');
	}
	exec(
	sprintf(
	'%s %s analyse --no-progress --level 8 --configuration %s --error-format json %s',
	escapeshellarg(PHP_BINARY),
	$phpstanBinPath,
	escapeshellarg(__DIR__ . '/phpstan.neon'),
	escapeshellarg(__DIR__ . '/data')
	),
	$output,
	$statusCode
	);
	$stringOutput = implode("\n", $output);
	$json = \Nette\Utils\Json::decode($stringOutput, \Nette\Utils\Json::FORCE_ARRAY);
	return [$statusCode, $json['files']];
	}

and I think thats kind of the point of this PR. the top level bash scripts make the overall picture more obvious then the pretty lengthy phpunit test before

[thg2k]

You are right, it is the same behavior as before, but my comment still stands.. wasn't it broken from the beginning?

[staabm]

you might be right. I don't know the specifics of the bug which this test is covering.

if you are right, it would mean the test-as it was before this PR did not reproduce the actual bug.

---

edit: the test might have worked initially and got broken with a later re-work in 9088b84#diff-18b49be48273139575cfe8bdb4075a2f4fe1ac1a26debeba8563b3e477ba916d

[ondrejmirtes]

This look like a nice opportunity to start using https://bashunit.typeddevs.com/, what do you think?

[staabm]

I am not sure its worth it.

Would the goal be running the tests in a single job?

We would need to start clearing temp folder etc, so it will get more complicated.

Also not sure the assertions would be more readable.

Could you give me a few more directions what you would expect?

[staabm]

I will start a new PR to give bashunit a try. this one here should be ready to land.

[ondrejmirtes]

+56 −186... yeah, this tells me this is much better than doing it in PHPUnit :)

[ondrejmirtes]

Thanks. I want to use "assert_contains" instead of opaque "grep" call I don't understand 😊