nuwave / lighthouse

A framework for serving GraphQL from Laravel
https://lighthouse-php.com
MIT License
3.37k stars 438 forks source link

Add automated tests for Lumen #123

Closed olivernybroe closed 3 years ago

olivernybroe commented 6 years ago

I think we need to setup automated tests to run on Lumen also. It is too often we end up breaking Lumen, because we don't have any tests telling us we did.

chrissm79 commented 6 years ago

@olivernybroe Couldn't agree more here.. Lumen has been a pain point w/ Lighthouse development.

Does anyone have an example of how to incorporate Travis CI w/ Lumen so we can put it through the tests?

hailwood commented 6 years ago

@chrissm79, Do we even have a setup for running Lumen tests locally? As far as I am aware orchestra/testbench doesn't support lumen, so we probably need to get that up and running first, at which point the travis setup should become fairly trivial

olivernybroe commented 6 years ago

@4levels do you have any idea how to make tests on Lumen?

spawnia commented 6 years ago

https://github.com/orchestral/testbench/issues/99

According to this issue, orchestra/testbench actually does not support Lumen - and they are not planning to do so in the near future.

I think we are probably better off just cutting the cord on Lumen. As i see it, using Lumen over Laravel is done for performance reasons. If you have a strong focus on performance, then Lighthouse is not the right tool anyways. We are optimizing for ease of development and maintainability.

@chrissm79 what do you think? My opinion is that there are still more important issues left to solve, i would close this for now and maybe come back to it later.

4levels commented 6 years ago

Hi all,

as far as I know, Laravel is full featured web framework, while Lighthouse is an API only library, just as Lumen. So imo it makes more sense for Lighthouse to be targeting Lumen first instead of Laravel, as in general all Lumen based libraries work in Laravel as well out of the box.
So I'm strongly convinced Lighthouse is better off dropping all dependencies on Laravel-only features and instead should focus on being fully Lumen compatible instead.

Just my 2 cents..

spawnia commented 6 years ago

While in principle, it would be nice to support Lumen, i see this issue out of a very pragmatic perspective.

Right now, we are heavily using Eloquent, Facades and probably some other, non-Lumen features that i can not think of right now. Plus, most importantly, our test setup only supports Laravel right now.

As of now, i think there are still a whole bunch of features that we require and issues that need to be resolved. Given that we have limited time and efforts to dispense on this library, i say we drop Lumen for now.

@4levels If you are willing to put in the work and your expertise, i am open to supporting Lumen.

4levels commented 6 years ago

Hi @spawnia,

so far Lighthouse works fine with Lumen, it seems only the testbench suite is not playing nice. From what I can read from the Lumen docs about testing, it supports all tools and features for testing Lighthouse out of the box.

Regarding Facades: this doesn't belong in a library like lighthouse anyway since they are userland helpers. Even in Laravel Facades are not required and can be turned off. And honestly, besides some syntactic sugar, they don't provide any real benefits anyway (I consider them even harmfull since they obfuscate a lot)

Eloquent is fully supported in Lumen, just as queues, jobs, events etc etc are.

I honestly am not familiar with Testbench, but as far as I can see from a quick look, why is Testbench even needed for an api only library like lighthouse, that already works without Facades? In Testbench's own readme, they advise to start off with reading the Laravel package development documentation, which in turn suggests to use Testbench if you rely on Facades (but we don't)..

My conclusion: Lighthouse doesn't need facades, so it doesn't need Testbench, it even doesn't need Laravel.

I'm very sad to not be able to spend more time on this, I'd love to adjust the test setup with vanilla Lumen test tools (that will work in Laravel as well).

Hope this all makes sense..

olivernybroe commented 6 years ago

I agree with @4levels in some degree. I think we should remove dependency on Laravel, and dont use Testbench, because we can easily work around it and not use it. This would also mean that we don't actually depend on Laravel, but we depend on some of Laravel's packages. Then we just only depend on the packages which Lumen has support for. By doing this we have support for all frameworks which has support for the packages, in our case the most important ones, Laravel and Lumen.

The biggest problem I see is that some stuff doesn't work the same way in Laravel and Lumen, even though they should. That is one of the biggest issues. I can totally understand that when we develop new stuff for Lighthouse, we do not want to do a manual test of all the features in Lumen, to make sure it actually works, because it does not behave exactly like Laravel.

e200 commented 5 years ago

I'm actually using Lighthouse with Lumen in production and will love to see some support on it.

Your package should not be highly coupled to Laravel while the real API purpose framework isn't being supported.

More support on Lumen. Consider also have 2 different services providers, one for Laravel and other for Lumen (LumenLighthouseServiceProvider).

spawnia commented 5 years ago

@e200 I invite you to add support for it yourself if Lumen support is something you require or deem important. I don't have plans to add it, but am open to taking contributions.

spawnia commented 4 years ago

There is a fork of Orchestra/testbench available, that might help: https://github.com/UnicornGlobal/lumen-testbench-core/tree/3915aeb8c11caba7dc7389b601a8f0421de1ac3a

olivernybroe commented 3 years ago

@spawnia I will close this one.

I don't think we have any intention of adding lumen testing as it seems like there isn't any easy way of doing that.