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

#[RequiresEnvironmentVariable] #6074

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions src/Framework/Attributes/RequiresEnvironmentVariable.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php declare(strict_types=1);
/*
* This file is part of PHPUnit.
*
* (c) Sebastian Bergmann <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/
namespace PHPUnit\Framework\Attributes;

use Attribute;

/**
* @immutable
*
* @no-named-arguments Parameter names are not covered by the backward compatibility promise for PHPUnit
*/
#[Attribute(Attribute::TARGET_CLASS | Attribute::TARGET_METHOD | Attribute::IS_REPEATABLE)]
final readonly class RequiresEnvironmentVariable
{
private string $environmentVariableName;
private null|bool|int|string $value;
sebastianbergmann marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@mvorisek mvorisek Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Environment variable can be strictly only string.

What is the meaning of null, bool, int?

Can I test environment variable presence/absence /wo specific value?

Copy link
Contributor

@mvorisek mvorisek Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I test environment variable presence/absence /wo specific value?

you can only test environment variable presence or with a specific value

it seems bool and int should be removed and as never possible

this was more or less my concern here. I'm also wondering if we should only check the env var in $_ENV , or use getenv() or check at least one of them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any thoughts @sebastianbergmann? I can provide a patch to my PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was more or less my concern here. I'm also wondering if we should only check the env var in $_ENV , or use getenv() or check at least one of them

Please test the behaviour with E2E test and some tests in separate process. The separate process must see the same environment variables in both linux and Windows.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think in the case of "no value", I check if the env var exist, or should I check it is ok-ish? empty string and '0' would not pass

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion:

  • If an environment variable is tested for a presence (/wo value defined in the Attribute), I would expect '0' to be consideted as set.

  • For binary '1'/'0' detection I would expect user to write #[RequiresEnvironmentVariable('foo', '1')] explicitly.

  • '' should be considered as unset as in many operating systems FOO= unsets the variable. Also, in CI parametrization/matrix empty value might be set, but with "no variable" meaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'' should be considered as unset as in many operating systems FOO= unsets the variable

yeah, good point


public function __construct(string $environmentVariableName, null|bool|int|string $value = null)
{
$this->environmentVariableName = $environmentVariableName;
$this->value = $value;
}

public function environmentVariableName(): string
{
return $this->environmentVariableName;
}

public function value(): null|bool|int|string
{
return $this->value;
}
}
20 changes: 20 additions & 0 deletions src/Metadata/Api/Requirements.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use const PHP_VERSION;
use function addcslashes;
use function array_column;
use function array_key_exists;
use function assert;
use function extension_loaded;
use function function_exists;
Expand All @@ -24,6 +25,7 @@
use function preg_match;
use function sprintf;
use PHPUnit\Metadata\Parser\Registry;
use PHPUnit\Metadata\RequiresEnvironmentVariable;
use PHPUnit\Metadata\RequiresFunction;
use PHPUnit\Metadata\RequiresMethod;
use PHPUnit\Metadata\RequiresOperatingSystem;
Expand Down Expand Up @@ -105,6 +107,24 @@ public function requirementsNotSatisfiedFor(string $className, string $methodNam
}
}

if ($metadata->isRequiresEnvironmentVariable()) {
assert($metadata instanceof RequiresEnvironmentVariable);

if (!array_key_exists($metadata->environmentVariableName(), $_ENV)) {
$notSatisfied[] = sprintf('Environment variable "%s" is required.', $metadata->environmentVariableName());

continue;
}

if ($metadata->value() !== null && $_ENV[$metadata->environmentVariableName()] !== $metadata->value()) {
$notSatisfied[] = sprintf(
'Environment variable "%s" is required to be "%s".',
$metadata->environmentVariableName(),
$metadata->value(),
);
}
}

if ($metadata->isRequiresOperatingSystemFamily()) {
assert($metadata instanceof RequiresOperatingSystemFamily);

Expand Down
18 changes: 18 additions & 0 deletions src/Metadata/Metadata.php
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,16 @@ public static function requiresPhpunitExtensionOnMethod(string $extensionClass):
return new RequiresPhpunitExtension(self::METHOD_LEVEL, $extensionClass);
}

public static function requiresEnvironmentVariableOnClass(string $environmentVariableName, null|int|string $value): RequiresEnvironmentVariable
{
return new RequiresEnvironmentVariable(self::CLASS_LEVEL, $environmentVariableName, $value);
}

public static function requiresEnvironmentVariableOnMethod(string $environmentVariableName, null|int|string $value): RequiresEnvironmentVariable
{
return new RequiresEnvironmentVariable(self::METHOD_LEVEL, $environmentVariableName, $value);
}

/**
* @param non-empty-string $setting
* @param non-empty-string $value
Expand Down Expand Up @@ -816,6 +826,14 @@ public function isRequiresPhpunitExtension(): bool
return false;
}

/**
* @phpstan-assert-if-true RequiresEnvironmentVariable $this
*/
public function isRequiresEnvironmentVariable(): bool
{
return false;
}

/**
* @phpstan-assert-if-true RequiresSetting $this
*/
Expand Down
10 changes: 10 additions & 0 deletions src/Metadata/MetadataCollection.php
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,16 @@ public function isRequiresPhpunitExtension(): self
);
}

public function isRequiresEnvironmentVariable(): self
{
return new self(
...array_filter(
$this->metadata,
static fn (Metadata $metadata): bool => $metadata->isRequiresEnvironmentVariable(),
),
);
}

public function isRequiresSetting(): self
{
return new self(
Expand Down
21 changes: 21 additions & 0 deletions src/Metadata/Parser/AttributeParser.php
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
use PHPUnit\Framework\Attributes\PostCondition;
use PHPUnit\Framework\Attributes\PreCondition;
use PHPUnit\Framework\Attributes\PreserveGlobalState;
use PHPUnit\Framework\Attributes\RequiresEnvironmentVariable;
use PHPUnit\Framework\Attributes\RequiresFunction;
use PHPUnit\Framework\Attributes\RequiresMethod;
use PHPUnit\Framework\Attributes\RequiresOperatingSystem;
Expand Down Expand Up @@ -315,6 +316,16 @@ public function forClass(string $className): MetadataCollection

break;

case RequiresEnvironmentVariable::class:
assert($attributeInstance instanceof RequiresEnvironmentVariable);

$result[] = Metadata::requiresEnvironmentVariableOnClass(
$attributeInstance->environmentVariableName(),
$attributeInstance->value(),
);

break;

case RequiresSetting::class:
assert($attributeInstance instanceof RequiresSetting);

Expand Down Expand Up @@ -691,6 +702,16 @@ public function forMethod(string $className, string $methodName): MetadataCollec

break;

case RequiresEnvironmentVariable::class:
assert($attributeInstance instanceof RequiresEnvironmentVariable);

$result[] = Metadata::requiresEnvironmentVariableOnMethod(
$attributeInstance->environmentVariableName(),
$attributeInstance->value(),
);

break;

case RequiresSetting::class:
assert($attributeInstance instanceof RequiresSetting);

Expand Down
44 changes: 44 additions & 0 deletions src/Metadata/RequiresEnvironmentVariable.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?php declare(strict_types=1);
/*
* This file is part of PHPUnit.
*
* (c) Sebastian Bergmann <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/
namespace PHPUnit\Metadata;

/**
* @immutable
*
* @no-named-arguments Parameter names are not covered by the backward compatibility promise for PHPUnit
*/
final readonly class RequiresEnvironmentVariable extends Metadata
{
private string $environmentVariableName;
private null|bool|int|string $value;

public function __construct(int $level, string $environmentVariableName, null|bool|int|string $value)
{
parent::__construct($level);

$this->environmentVariableName = $environmentVariableName;
$this->value = $value;
}

public function isRequiresEnvironmentVariable(): true
{
return true;
}

public function environmentVariableName(): string
{
return $this->environmentVariableName;
}

public function value(): null|bool|int|string
{
return $this->value;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php declare(strict_types=1);
/*
* This file is part of PHPUnit.
*
* (c) Sebastian Bergmann <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/
namespace PHPUnit\TestFixture\Metadata\Attribute;

use PHPUnit\Framework\Attributes\RequiresEnvironmentVariable;
use PHPUnit\Framework\TestCase;

#[RequiresEnvironmentVariable('foo', 'bar')]
final class RequiresEnvironmentVariableTest extends TestCase
{
#[RequiresEnvironmentVariable('foo')]
#[RequiresEnvironmentVariable('bar', 'baz')]
public function testOne(): void
{
}
}
23 changes: 23 additions & 0 deletions tests/_files/RequirementsEnvironmentVariableTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php
declare(strict_types=1);
/*
* This file is part of PHPUnit.
*
* (c) Sebastian Bergmann <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/
namespace PHPUnit\TestFixture;

use PHPUnit\Framework\Attributes\RequiresEnvironmentVariable;
use PHPUnit\Framework\TestCase;

final class RequirementsEnvironmentVariableTest extends TestCase
{
#[RequiresEnvironmentVariable('foo', 'bar')]
#[RequiresEnvironmentVariable('baz')]
public function testRequiresEnvironmentVariable(): void
{
}
}
20 changes: 20 additions & 0 deletions tests/unit/Metadata/Api/RequirementsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use PHPUnit\Framework\Attributes\Group;
use PHPUnit\Framework\Attributes\Small;
use PHPUnit\Framework\TestCase;
use PHPUnit\TestFixture\RequirementsEnvironmentVariableTest;

#[CoversClass(Requirements::class)]
#[Small]
Expand Down Expand Up @@ -130,6 +131,12 @@ public static function missingRequirementsProvider(): array
];
}

protected function tearDown(): void
{
parent::tearDown();
unset($_ENV['foo']);
}

#[DataProvider('missingRequirementsProvider')]
public function testGetMissingRequirements(string $test, array $result): void
{
Expand All @@ -138,4 +145,17 @@ public function testGetMissingRequirements(string $test, array $result): void
(new Requirements)->requirementsNotSatisfiedFor(\PHPUnit\TestFixture\RequirementsTest::class, $test),
);
}

public function testGetMissingEnvironmentVariableRequirements(): void
{
$_ENV['foo'] = '';

$this->assertEquals(
[
'Environment variable "foo" is required to be "bar".',
'Environment variable "baz" is required.',
],
(new Requirements)->requirementsNotSatisfiedFor(RequirementsEnvironmentVariableTest::class, 'testRequiresEnvironmentVariable'),
);
}
}
10 changes: 10 additions & 0 deletions tests/unit/Metadata/MetadataCollectionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
#[UsesClass(RequiresPhpExtension::class)]
#[UsesClass(RequiresPhpunit::class)]
#[UsesClass(RequiresPhpunitExtension::class)]
#[UsesClass(RequiresEnvironmentVariable::class)]
#[UsesClass(RequiresSetting::class)]
#[UsesClass(RunClassInSeparateProcess::class)]
#[UsesClass(RunInSeparateProcess::class)]
Expand Down Expand Up @@ -402,6 +403,14 @@ public function test_Can_be_filtered_for_RequiresPhpunitExtension(): void
$this->assertTrue($collection->asArray()[0]->isRequiresPhpunitExtension());
}

public function test_Can_be_filtered_for_RequiresEnvironmentVariable(): void
{
$collection = $this->collectionWithOneOfEach()->isRequiresEnvironmentVariable();

$this->assertCount(1, $collection);
$this->assertTrue($collection->asArray()[0]->isRequiresEnvironmentVariable());
}

public function test_Can_be_filtered_for_RequiresSetting(): void
{
$collection = $this->collectionWithOneOfEach()->isRequiresSetting();
Expand Down Expand Up @@ -553,6 +562,7 @@ private function collectionWithOneOfEach(): MetadataCollection
),
),
Metadata::requiresPhpunitExtensionOnClass(stdClass::class),
Metadata::requiresEnvironmentVariableOnClass('foo', 'bar'),
Metadata::requiresSettingOnClass('foo', 'bar'),
Metadata::runClassInSeparateProcess(),
Metadata::runInSeparateProcess(),
Expand Down
Loading
Loading