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

Add ability to change DB host and port for tests #286

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

vjik
Copy link
Member

@vjik vjik commented Jun 23, 2023

Q A
Is bugfix?
New feature?
Breaks BC?
Fixed issues -

@what-the-diff
Copy link

what-the-diff bot commented Jun 23, 2023

PR Summary

  • Introduced TestEnvironment class
    Added a new file TestEnvironment.php in the tests/Support/ directory, with methods to retrieve PostgreSQL host and port. This centralizes the access to these values.

  • Modified CommandTest.php
    Using the new TestEnvironment class to retrieve PostgreSQL host and port for DSN instantiation and assertions in the testShowDatabases() method.

  • Modified PDODriverTest.php
    Using the new TestEnvironment class to retrieve PostgreSQL host and port for driver instantiation in the testConnectionCharset() method.

  • Modified TestTrait.php
    Using the new TestEnvironment class to retrieve PostgreSQL host and port in the methods getConnection, getDb, and getDsn. This refactoring centralizes the way these values are used across multiple methods.

@vjik vjik requested a review from a team June 23, 2023 13:15
@vjik vjik added the status:code review The pull request needs review. label Jun 23, 2023
@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (ada7f05) 99.67% compared to head (5fd76d8) 99.67%.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #286   +/-   ##
=========================================
  Coverage     99.67%   99.67%           
  Complexity      213      213           
=========================================
  Files            13       13           
  Lines           617      617           
=========================================
  Hits            615      615           
  Misses            2        2           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@terabytesoftw
Copy link
Member

terabytesoftw commented Jun 23, 2023

What is the use case for this change, tests must be run with the default values.

@vjik
Copy link
Member Author

vjik commented Jun 23, 2023

What is the use case for this change?

Usage two container for tests (PHP and DB) in one network. In this case I need change host to actual.

@terabytesoftw
Copy link
Member

What is the use case for this change?

Usage two container for tests (PHP and DB) in one network. In this case I need change host to actual.

This is not a problem with the package, it is a problem with your configuration.

@vjik
Copy link
Member Author

vjik commented Jun 23, 2023

What is the use case for this change?

Usage two container for tests (PHP and DB) in one network. In this case I need change host to actual.

This is not a problem with the package, it is a problem with your configuration.

It's package problem that deny change host address.

@terabytesoftw
Copy link
Member

What is the use case for this change?

Usage two container for tests (PHP and DB) in one network. In this case I need change host to actual.

This is not a problem with the package, it is a problem with your configuration.

It's package problem that deny change host address.

Imagine that each user, make a pr to adapt the package to its configuration, for me it should look like this, simple configuration.

@vjik vjik added the type:test Test label Jun 23, 2023
docs/en/testing.md Outdated Show resolved Hide resolved
docs/en/testing.md Outdated Show resolved Hide resolved
docs/en/testing.md Outdated Show resolved Hide resolved
docs/en/testing.md Outdated Show resolved Hide resolved
vjik and others added 4 commits June 25, 2023 18:44
Co-authored-by: Alexander Makarov <[email protected]>
Co-authored-by: Alexander Makarov <[email protected]>
Co-authored-by: Alexander Makarov <[email protected]>
Co-authored-by: Alexander Makarov <[email protected]>
docs/en/testing.md Outdated Show resolved Hide resolved
docs/en/testing.md Outdated Show resolved Hide resolved
docs/en/testing.md Outdated Show resolved Hide resolved
{
public static function getPostgreSqlHost(): string
{
$host = getenv('YIISOFT_DB_PGSQL_TEST_HOST');
Copy link
Contributor

Choose a reason for hiding this comment

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

need use const

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that is necessary since that's the only place this string is used.

@vjik vjik added status:under development Someone is working on a pull request. and removed status:code review The pull request needs review. labels Jun 25, 2023
@vjik vjik assigned vjik and darkdef Jun 25, 2023
docs/en/testing.md Outdated Show resolved Hide resolved
docs/en/testing.md Outdated Show resolved Hide resolved
docs/en/testing.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:under development Someone is working on a pull request. type:test Test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants