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

Moved tests to their own kernel #654

Closed

Conversation

circulon
Copy link
Contributor

@circulon circulon commented Jun 2, 2022

This allows tests/* to be removed from production packaging without crashing the framework
Also moved dev requirements out of requirements.txt

Copy link
Contributor

@girardinsamuel girardinsamuel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand the motivation for moving tests to their own Kernel.

This allows tests/* to be removed from production packaging without crashing the framework

What is the benefit here ? A smaller package ? Should not we just remove tests folder from packaging configuration ? 🤔 Can you clarify a bit ? 🙏

requirements-dev.txt Outdated Show resolved Hide resolved
@@ -11,7 +12,7 @@
"""

application.register_providers(
Kernel,
TestsKernel,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here Kernel was the base Kernel used to define the minimal specifications for the framework, and ApplicationKernel used to define project-specific and feature configuration. I think we should not remove this.

If you want to split the tests part maybe we should move it to its own TestsKernel, without removing Kernel and then TestsKernel would contain only the register_testing() method..

But I am not sure about what you want to split this and why you want to remove the tests/ folder ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here Kernel was the base Kernel used to define the minimal specifications for the framework, and ApplicationKernel used to define project-specific and feature configuration. I think we should not remove this.

Hi @girardinsamuel

This is correct, but the problem with this minimum config is that it incorporates the tests into the main runtime even if not doing tests.

from ..tests.HttpTestResponse import HttpTestResponse
from ..tests.TestResponseCapsule import TestResponseCapsule

This means that you can never slim down the package (for production deployment) by removing the tests folder.
Currently I use this method (of stripping tests and other unused files from the package to create pre-compiled AWS Lambda Layers. But I cannot strip the tests from Masonite due to their integration point in the Kernel
I hope this clarifies the usage scenario for you

If you want to split the tests part maybe we should move it to its own TestsKernel, without removing Kernel and then TestsKernel would contain only the register_testing() method..

Yeah that makes sense. I was just trying to keep the same functionality but if we dont need all that for the tests then thats much cleaner.
... Let me try this and make sure I can get it working

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here Kernel was the base Kernel used to define the minimal specifications for the framework, and ApplicationKernel used to define project-specific and feature configuration. I think we should not remove this.

The point of altering this file was that it is ONLY used for tests
ie the basepath is set to the tests folder and ApplicationKernel ia imported from tests too

from tests.integrations.config.providers import PROVIDERS
from tests.integrations.app.Kernel import Kernel as ApplicationKernel

# here the project base path is tests/integrations
application = Application(base_path("tests/integrations"))

Thia file would not obviously be used when building a starting app

@josephmancuso
Copy link
Member

Trying to follow along here, is this an issue that needs to be solved? I'm not sure of the actual problem here

@girardinsamuel
Copy link
Contributor

Trying to follow along here, is this an issue that needs to be solved? I'm not sure of the actual problem here

That's not an issue (in the sense of a bug). It's a proposal to split tests feature from the rest of the framework to allow packaging a smallest bundle in production if I understand correctly.
Right now tests feature is added inside base masonite Kernel class. @circulon propose to put this in its own Kernel to allow not loading test feature (and not having to have pytest installed).

@circulon
Copy link
Contributor Author

circulon commented Jun 2, 2022

Trying to follow along here, is this an issue that needs to be solved? I'm not sure of the actual problem here

That's not an issue (in the sense of a bug). It's a proposal to split tests feature from the rest of the framework to allow packaging a smallest bundle in production if I understand correctly. Right now tests feature is added inside base masonite Kernel class. @circulon propose to put this in its own Kernel to allow not loading test feature (and not having to have pytest installed).

@girardinsamuel @josephmancuso
Yes, that is exactly what I meant with this.
Apolofies for the confusion

@josephmancuso
Copy link
Member

If thats the case then this won't be able to be done in 4.0 because it would be a breaking change. Everyone would need to add the provider to get the testing features

@girardinsamuel
Copy link
Contributor

If thats the case then this won't be able to be done in 4.0 because it would be a breaking change. Everyone would need to add the provider to get the testing features

I guess we can study this for Masonite 5 then !

@circulon
Copy link
Contributor Author

circulon commented Jun 4, 2022

If thats the case then this won't be able to be done in 4.0 because it would be a breaking change. Everyone would need to add the provider to get the testing features

@josephmancuso
Why would this be a breaking change?
Why would the tests need to be a Provider at all?

The tests work right now and the main Kernel works fine without them in it.
for those devs that are doing automated tests the wsgi.py at the root works fine and demonstrates how to alter their inline testing by adding the TestsKernel separately, like so

application.register_providers(
    Kernel,
    TestsKernel,
    ApplicationKernel,
)

Can you please explain why this would be a Breaking Change please?

@girardinsamuel
Copy link
Contributor

If thats the case then this won't be able to be done in 4.0 because it would be a breaking change. Everyone would need to add the provider to get the testing features

@josephmancuso Why would this be a breaking change? Why would the tests need to be a Provider at all?

The tests work right now and the main Kernel works fine without them in it. for those devs that are doing automated tests the wsgi.py at the root works fine and demonstrates how to alter their inline testing by adding the TestsKernel separately, like so

application.register_providers(
    Kernel,
    TestsKernel,
    ApplicationKernel,
)

Can you please explain why this would be a Breaking Change please?

Breaking change would be that existing Masonite 4 projects would not have this modification inside their wsgi.py file !

@josephmancuso
Copy link
Member

Yeah the breaking change is because existing applications would have to add this kernel file to their application containers manually and if they don't then all their tests would break. https://github.com/MasoniteFramework/cookie-cutter/blob/4.0/wsgi.py#L11

Sorry I think this PR is just throwing too much of a wrench into the mix right now when its not really solving an issue.

I like the design and you are right about keeping each feature of the framework standardized but I just don't feel comfortable making this change right now. Maybe when Masonite 5 branches start opening we can revisit this

@circulon
Copy link
Contributor Author

circulon commented Nov 9, 2022

@josephmancuso @girardinsamuel
Now I see that there is movement towards 5.0
just a reminder to include this PR

or at least incorporate its ideas into 5.x
Cheers

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.

None yet

3 participants