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

Setup colors in TestCase #441

Closed
wants to merge 1 commit into from
Closed

Setup colors in TestCase #441

wants to merge 1 commit into from

Conversation

spaze
Copy link

@spaze spaze commented Apr 15, 2023

  • bug fix / new feature? bug fix (I'm running Tester 2.5.0)
  • BC break? no

If not set up, "Typed static property Tester\Environment::$useColors must not be accessed before initialization in .../Framework/Environment.php:267" is thrown when \Tester\Environment::setup() is not called in the TestCase-based test file.

Without the patch:

$ vendor/nette/tester/src/tester -c tests/php-unix.ini tests/
 _____ ___  ___ _____ ___  ___
|_   _/ __)( __/_   _/ __)| _ )
  |_| \___ /___) |_| \___ |_|_\  v2.5.0

PHP 8.2.5 (cli) | php -n -c [...]/php-unix.ini | 8 threads

............F

-- FAILED: ConfigExtensionTest.phpt
   Exited with error code 255 (expected 0)

   Fatal error: Uncaught Error: Typed static property Tester\Environment::$useColors must not be accessed before initialization in [...]vendor/nette/tester/src/Framework/Environment.php:267
   Stack trace:
   #0 [...]vendor/nette/tester/src/Framework/TestCase.php(63): Tester\Environment::print('\e[1m\e[31m\xC3\x97\e[0m...')
   #1 [...]tests/ConfigExtensionTest.phpt(85): Tester\TestCase->run()
   #2 {main}
     thrown in [...]vendor/nette/tester/src/Framework/Environment.php on line 267


FAILURES! (13 tests, 1 failure, 1.6 seconds)

With the patch:

$ vendor/nette/tester/src/tester -c tests/php-unix.ini tests/
 _____ ___  ___ _____ ___  ___
|_   _/ __)( __/_   _/ __)| _ )
  |_| \___ /___) |_| \___ |_|_\  v2.5.0

PHP 8.2.5 (cli) | php -n -c [...]/php-unix.ini | 8 threads

............F

-- FAILED: ConfigExtensionTest.phpt
   Exited with error code 255 (expected 0)
   √ testService
   × testConfig
[...]

Adding \Tester\Environment::setup(); to the testcase file also fixes the problem, for example:

// [...]
require __DIR__ . '/../vendor/autoload.php';
\Tester\Environment::setup();

class ConfigExtensionTest extends TestCase
// [...]

But I believe calling it should not be necessary (I have seen the Environment::setup() notes in the README and the docs).

I'm a Tester internals noob and am not sure if this is the right way, let me know if that's not the case. I've added the call because I've seen a similar call in here:

Environment::setupColors();

If not set up, "Typed static property Tester\Environment::$useColors must not be accessed before initialization in .../Framework/Environment.php:267" is thrown when `\Tester\Environment::setup()` is not called in the TestCase file.
spaze added a commit to spaze/csp-config that referenced this pull request Apr 15, 2023
spaze added a commit to spaze/nonce-generator that referenced this pull request Apr 15, 2023
@grogy
Copy link
Contributor

grogy commented May 1, 2023

Hello @spaze I confirm the error. I am not sure the solution. For it I created alternative in #442

@spaze
Copy link
Author

spaze commented May 1, 2023

I don't know if this is the right way either but I went with this patch because the default might not be "no colors" which is what setting

public static bool $useColors = false;

would do.

The default may be "use colors" especially when using env vars like FORCE_COLOR, which is what's resolved in Environment::setupColors();

@grogy
Copy link
Contributor

grogy commented May 2, 2023

I don't think so.

TestCase can be called without someone calling Environment::setup() or Environmnetn::setupColors() first. In doing so, the Environment::print() method is called where the property is just read and not used. I mean line:

fwrite(STDOUT, self::$useColors ? $s : Dumper::removeColors($s));

David will know more. I'm looking more closely into the guts for the first time :-)

@spaze
Copy link
Author

spaze commented May 2, 2023

Yeah, this is the bug this PR is trying to fix 😊

@milo
Copy link
Member

milo commented May 4, 2023

I've fixed this by #442. Calling Environment::setup*() is recommended, but optional.

@milo milo closed this May 4, 2023
@spaze
Copy link
Author

spaze commented May 4, 2023

Environment::setupColors(); is always called in CliTester 😊 Defaulting to false disables colors in some cases (like calling the testcase directly), even if using FORCE_COLOR=1 for example, which seems inconsistent with how the rest of the package works (updated: it's actually consistent, sorry).

@spaze spaze deleted the patch-1 branch May 4, 2023 12:16
@milo
Copy link
Member

milo commented May 4, 2023

CliTester is a test runner and it calls setupColors() for own purpose. Tests itself run is a different process and CliTester only prepares environment for them. It is up to you call Environment::setup() when you with to use it.

@spaze
Copy link
Author

spaze commented May 4, 2023

Thanks, that makes sense, adding the initial value is what fixes the original problem, understand the distinction now :-) This PR would fix the problem "just by the way", by changing something else, which doesn't indeed seem correct.

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.

3 participants