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

feat: create attribute #[WithEnvironmentVariable] #6126

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nikophil
Copy link
Contributor

@nikophil nikophil commented Feb 9, 2025

fixes #6116

foreach ($environmentVariables as $environmentVariableName => $environmentVariableValue) {
assert($metadata instanceof WithEnvironmentVariable);

$this->backupEnvironmentVariables[$environmentVariableName] = $_ENV[$environmentVariableName] ?? getenv($environmentVariableName);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was torn on this... in PHP, nothing prevents to have a different value for $_ENV['VAR'] and getenv('VAR').
Do you think I should store both representations, in order to have 100% the same environment afterand before the test?

Choose a reason for hiding this comment

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

I think so, yes.

Copy link
Contributor Author

@nikophil nikophil Feb 11, 2025

Choose a reason for hiding this comment

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

any hints how I should store this?
my first guess would be to store this in an array:

    /**
     * @var array{getenv: array<string, false|string>, global: array<string, false|string>}
     */
    private array $backupEnvironmentVariables = ['getenv' => [], 'global'  => []];

but I don't like it. Another idea would be to use a value object:

final readonly class BackupEnvironmentVariable
{
    public static function asGetenv(): self;
    public static function asSuperGlobal(): self;    
}

if so, in which namespace should i put this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the code to use a value object. (some tests are failing, I'll check them after we agree on the implementation)

Comment on lines 108 to 111
#[WithEnvironmentVariable('BAZ', '1')]
#[WithEnvironmentVariable('BAZ', '2')]
#[WithEnvironmentVariable('BAZ', '3')]
public function testMultipleAttributesKeepTheLastValue(): void
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we should throw an exception for this case?

Copy link

codecov bot commented Feb 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.09%. Comparing base (de81d0e) to head (48d3ead).
Report is 36 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6126      +/-   ##
============================================
+ Coverage     95.08%   95.09%   +0.01%     
- Complexity     6738     6754      +16     
============================================
  Files           725      726       +1     
  Lines         21251    21306      +55     
============================================
+ Hits          20207    20262      +55     
  Misses         1044     1044              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 23 to 39
public static function setUpBeforeClass(): void
{
self::assertEnvironmentVariablesHaveDefaultValues();
}

protected function setUp(): void
{
$this->assertEnvironmentVariablesHaveCustomValues();
}

protected function tearDown(): void
{
$this->assertEnvironmentVariablesHaveCustomValues();
}

public static function tearDownAfterClass(): void
{
self::assertEnvironmentVariablesHaveDefaultValues();
}
Copy link
Contributor Author

@nikophil nikophil Feb 9, 2025

Choose a reason for hiding this comment

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

not sure of what would be the desired behavior here 🤔

Comment on lines +117 to +123
#[WithEnvironmentVariable('BAZ', '1')]
#[RequiresEnvironmentVariable('BAZ', '1')]
public function testUsingAlongWithRequiresEnvironmentVariableAttribute(): void
{
$this->assertSame('1', $_ENV['BAZ']);
$this->assertSame('1', getenv('BAZ'));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really sure how these two attributes should interact...

that's a bit odd to use them both on the same test, tho...

@nikophil nikophil changed the title feat: create attributei #[WithEnvironmentVariable] feat: create attribute #[WithEnvironmentVariable] Feb 9, 2025
@sebastianbergmann sebastianbergmann added this to the PHPUnit 12.1 milestone Feb 10, 2025
@sebastianbergmann sebastianbergmann added type/enhancement A new idea that should be implemented feature/test-runner CLI test runner feature/metadata/attributes labels Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/metadata/attributes feature/test-runner CLI test runner type/enhancement A new idea that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide #[WithEnvironmentVariable()] attribute to set env variables
2 participants