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

CiviUnitTestCase - Extract Api3TestTrait and Api3DocTrait #11872

Merged
merged 2 commits into from
Mar 24, 2018

Conversation

totten
Copy link
Member

@totten totten commented Mar 23, 2018

Overview

This is a simple refactoring which moves several utility functions out of the base-class CiviUnitTestCase and into smaller traits, Api3TestTrait and Api3DocTrait.

Before

  • CiviUnitTestCase is a 4000-line behemoth.

After

  • CiviUnitTestCase is a 3600-line behemoth, which builds on Api3TestTrait and Api3DocTrait.
  • Other PHPUnit tests can incorporate Api3TestTrait if they want to easily perform assertions about API calls.
  • Other PHPUnit tests can incorporate Api3DocTrait if they want to easily perform assertions about API calls and also make example files from the API call log. (Personally, I don't expect to use Api3DocTrait, but it's better to move that bit to separate file.)

Comments

  • I spot-checked a few tests locally to ensure they still pass.
  • If the PR test passes, that's a pretty good sign that it works... considering that this refactoring is only test code.
  • Additionally, it would be good to write a quicky extension with a test which uses Api3TestTrait.

@eileenmcnaughton
Copy link
Contributor

@totten style issue

totten added 2 commits March 23, 2018 20:41
This makes `CiviUnitTestCase` more readable, and it allows other tests to
benefit from these helpers (even if they don't use everything from
`CiviUnitTestCase`).
This makes `CiviUnitTestCase` more readable, and it allows other tests to
benefit from these helpers (even if they don't use everything from
`CiviUnitTestCase`).
@totten totten force-pushed the master-test-trait branch from ffe9f41 to 195557d Compare March 24, 2018 03:42
@totten
Copy link
Member Author

totten commented Mar 24, 2018

Thanks. Fixed/squashed/pushed.

@eileenmcnaughton
Copy link
Contributor

Added merge on pass - Jenkins is the only review that matters for this one

@seamuslee001
Copy link
Contributor

Tests have passed and only affects unit tests merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants