-
Notifications
You must be signed in to change notification settings - Fork 20
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
Combine with silverstripe/serve instead of using TestSessionRequestFilter #42
Comments
Related discussion here silverstripe/silverstripe-serve#2 about the PHP API that might come alongside these CLI tools. |
FYI I don't think that we should make it impossible to use other server stacks. For example, it would be interesting to have some kind of docker-serve analogue of silverstripe/serve. However, I don't think that we should have built-in hooks to initiate test sessions via a web request. Possibly, we could leave this code available in a place that's disabled by default. |
Well, I'm currently running behat against my dev container (which is a docker container with apache). The only thing I have to patch there for tests to pass is to increase |
The particular thing that is a bit icky right now is the creation-of and linking-to a test database. It's managed session variables and special hooks in the ORM just for this purpose. It's a bit gross. It would be nicer if switching to a different test session was done with an environment variable specifically set for the web-server spun up for the purpose of running behat tests against it. It would be great to do this with docker containers as well as Then you could use any server you like, provided there was an implementation of the interface that says how to start it, and we could provide a couple of options out of the box - one based on |
Oh, I see, I think I understand now. |
How would you see this working with e.g. |
Well, technically Apache is not a requirement for the framework. That means, On the other hand, people should be able to test their exact environment setup with end-to-end suites, so I reckon testsession should support working without serve anyway. |
This module serves two cases:
Serge appears to be focused on the former, Matt on the latter. To Matt I would say "well if you've built a complicate project with different needs, you're not obliged to use this". However, it would suggest that any refactoring as described above was optional rather than mandatory. I'd note that this ticket is a bit of a "hey wouldn't it be cool if..." idea of mine from 3 years ago, so I'd probably leave it to the creativity of Serge and others if they want to take a different approach for this. That said, you're also welcome to box on with it.
Another idea I think I had in the past was that it would set the env vars such as |
I've been working on using https://github.com/assertchris/silverstripe-serve to support more of the official SilverStripe build.
I think that we'd improve the usefulness and security of testsession if we coupled it more closely with serve, and did away with the TestSessionRequestFilter.
A new CLI tool such as
vendor/bin/serve-testsession --fixture=something.yml
could provide this. I'm not sure whether testsession should pull in serve as a dependency, or the other way around, but the former is probably the best.The script would do this:
The secret key could, for instance, be part of the bootstrap-file argument to silverstripe/serve: we could use a bootstrap file rather than applying TestSessionRequestFilter to set up the test session, which would reduce the security risk of this module if it got onto a production environment somehow. (See #21). Note that the bootstrap file is called between Constants.php and Core.php.
A secret key could also be supplied as an environment variable.
We'd lose the ability to access these testsessions through normal server configs, but that's a security improvement.
vendor/bin/serve-testsession
, with appropriate fixtures, could also potentially be a helpful tool for developers working in their local environment outside of test automation activities./cc @assertchris @chillu
The text was updated successfully, but these errors were encountered: