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

[PROPOSAL] Add more tests to the library #865

Merged
merged 10 commits into from
Jan 1, 2020
Merged

Conversation

mfn
Copy link
Collaborator

@mfn mfn commented Jan 1, 2020

I don't know how others feel, but many of my projects depend on this library.

Eventually I would like to contribute, give back something to this great idea of the author and the community. My PR #859 was merged and I really wanted to write a test, but I realized: there is nothing (ok, 2 tests…) but also no infrastructure for running these kind of tests (Laravel framework setup, models to test, etc;) 😢

Laravel packages whose functionality depends on the Laravel framework cannot easily be tested just with PHPUnit alone and usually require https://github.com/orchestral/testbench

So what I did in this PR

  • (chore: moved tests into a dedicated Tests namespace)
  • removed phpunit and added orchestral/testbench (which itselds depends on PHPUnit…)
  • picked one small piece of code and wrote a test for it, to get things kickstarted
  • reworked travis and
    • removed actual running unit tests on very old PHP/Laravel versions (see below)
    • only run phpcs in one combination (usually the latest)

Immediate issues I encountered

  • According to the composer.json version constraints, this library supports VERY OLD versions of Laravel
  • composer could not resolve any combination for orchestral/testbanch for the older Laravel versions, I got multi-page of package resolution errors
  • I could not find a sane way to keep the old tests, depending on PHPUnit_Framework_TestCase, working with the supported versions of orchestral because it depends on newer phpunit version now aliasing PHPUnit_Framework_TestCase to PHPUnit\Framework\Testcase anymore

Since I could not find a solution to this, I simply decided to:

  • remove running the tests for these old versions/combinations
  • but keep the old versions in travis because they still run composer install and ensure the combination itself works

What does this mean?

  • I think eventually at some point the version constraints should be cleaned up and non supported versions probably be dropped. But I don't want to make this call and not in this PR
  • I hope to make it easier for others to not just contribute features but also add tests for them

I certainly offer to add more tests to bring them up to speed, but before investing more time I would first like this approach to be approved, I hope this is understandable 😄

@barryvdh
Copy link
Owner

barryvdh commented Jan 1, 2020

I think we can bump the minimum version to laravel 5.5 (and the php version that supports that), or higher if that makes it easier.

@barryvdh
Copy link
Owner

barryvdh commented Jan 1, 2020

And nice work :)
Should I merge this or should we bump the versions and simplify it a bit?

@mfn
Copy link
Collaborator Author

mfn commented Jan 1, 2020

Merge is fine, I'll follow up with your suggestions!

@barryvdh barryvdh merged commit fb0c576 into barryvdh:master Jan 1, 2020
@mfn mfn deleted the mfn-phpunit branch January 1, 2020 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants