Unify run-tests.php test parsing

[mvorisek]

No description provided.

[devnexen]

looks nice simplification (redundant path :-)) cc @iluuu1994

[iluuu1994]

The strncmp() to rtrim($line, "\n\r") !== change makes sense to me. As for moving the check into the loop, the only thing this gets rid of is one fgets call at the cost of checking it every time, I'm not sure that makes sense.

[mvorisek]

also $this->sections[$section], newly the TEST section is parsed as all other sections...

[mvorisek]

@iluuu1994 can this PR be merged?

[iluuu1994]

@mvorisek I'm not too convinced the unification of the TEST section is an improvement. It's not really unified anyway (given the special if) but needlessly adds a condition for every line that can only be true for the first one. The behavioral change on the other hand makes sense (changing strncmp('--TEST--', $line, 8) to rtrim($line, "\n\r") !== '--TEST--'). I also find the removal of $secdone = false; confusing. I get it's not necessary but it's not immediately obvious that if (rtrim($line, "\n\r") !== '--TEST--') { guarantees its declaration.

I don't really care too much if others think this is fine, but I'd prefer just changing the --TEST-- on a separate line check and fixing the broken tests.

[mvorisek]

Previously the TEST section was parsed differently, && $this->sections[$section] was needed even if any section cannot be specified more than once, and ALLOWED_SECTIONS was missing the section. Now it is unified. I belive the change is improvement and it is a good practice to dedup a code. @devnexen welcome this change. Maybe we can ask someone else to give it a green light? @nielsdos? @MaxKellermann?

[MaxKellermann]

Sorry, I know nothing about the PHP unit tests, barely enough to be able to run them.

[nielsdos]

It's not up to me to decide any of this. :)
FWIW I think what iluuu1994 says here makes sense.

[derickr]

There is no consensus for merging this, so I am closing this PR. It also has conflicts.