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

Added support for configuring test db through DSN #1782

Merged
merged 2 commits into from
May 13, 2020
Merged

Conversation

othercorey
Copy link
Member

@othercorey othercorey commented May 11, 2020

This removes the large list of environment variables needed to configure a test and replaces with a single environment variable TEST_DSN. This dsn is parsed into TEST_DB_CONFIG which all the tests used to configure.

This also fixes MysqlAdapter and PostgresAdapter when user or pass is default.

Code coverage is updated to coveralls to match cakephp core and allow multiple builds to submit coverage.

@MasterOdin
Copy link
Member

For my local development workflow, this is going to make things a lot more annoying as now instead of just doing composer run test to test all adapters at once, I've got to run it four times while juggling the TEST_DSN variable.

@othercorey
Copy link
Member Author

Most test harnessed don't test multiple databases at the same time. You would normally iterate on one and final test on all.

However, if that is a hard requirement for phinx, we can split it up.

@dereuromark
Copy link
Member

I wonder if we can provide a high level entry point or composer script to test all at once locally.
Could also be something you put into your phpunit.xml and it will auto-read from it.

@MasterOdin btw: composer run test can just be used as composer test

@MasterOdin
Copy link
Member

MasterOdin commented May 11, 2020

final test on all

Yeah, that's what I meant by testing against all databases at once. My workflow is roughly:

  1. Make changes
  2. Run specific tests that I know are affected (be it specific test class, specific adapter, using --filter argument)
  3. Run all tests against all databases
  4. Run linting / static analysis

That third step now is a much greater nuisance to do now for the sake of CI cleanliness. It's also repeating several hundred tests per run that are not database specific just making the whole thing take longer. Ideally, I think that phpunit.xml should have DB specific variables (e.g. MYSQL_TEST_DSN, POSTGRES_TEST_DSN) that parse into DB specific configs (e.g. MYSQL_TEST_DB_CONFIG) that then activate the various adapter specific tests.

@MasterOdin btw: composer run test can just be used as composer test

Yeah, I know. I use npm a bunch and hate special cases, so I'm just always using the run command out of habit.

@othercorey othercorey force-pushed the test-config branch 2 times, most recently from e69cc48 to d339007 Compare May 11, 2020 21:46
@othercorey
Copy link
Member Author

Since this test suite only takes a few seconds, running a few extra tests isn't a concern.

To ease the transition, the dsn is now split into separate environment variables for each db:

  • SQLITE_DSN
  • MYSQL_DSN
  • PGSQL_DSN
  • SQLSRV_DSN

@dereuromark dereuromark merged commit 0d00f12 into master May 13, 2020
@dereuromark dereuromark deleted the test-config branch May 13, 2020 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants