search

openfaas / python-flask-template

HTTP and Flask-based OpenFaaS templates for Python 3
MIT License
85 stars 86 forks source link

Python function test #71

Open vidz1979 opened 8 months ago

vidz1979 commented 8 months ago

My actions before raising this issue

Expected Behaviour

Using python3-http template, there is a file handler_test.py. Modified it to not pass the test:

from .handler import handle

def test_handle():
    assert False

I've built the function and I was expecting to get an test error. Did a local run and also got no test errors.

Current Behaviour

I didn't get any type of test errors.

Possible Solution

Steps to Reproduce (for bugs)

  1. Write failing tests
  2. Build and get no errors.

Context

Testing code is essential. Providing a solid way to write and run tests is a must.

Your Environment

CLI: commit: f72db8de657001a2967582c535fe8351c259c5f6 version: 0.16.17

Next steps

You may join Slack for community support.

alexellis commented 8 months ago

@LucasRoesler would you have any links etc that you can share on this for @vidz1979 ?

@vidz1979 please edit and fill out the whole issue template

https://raw.githubusercontent.com/openfaas/python-flask-template/master/.github/ISSUE_TEMPLATE.md

LucasRoesler commented 8 months ago

@alexellis i just tested this, it is a bug, I can replicate it and the cause was this https://github.com/openfaas/python-flask-template/commit/79b444bca68df126b7d9e31aca35857dbd4d80f9

FROM build as test
ARG TEST_COMMAND=tox
ARG TEST_ENABLED=true
RUN [ "$TEST_ENABLED" = "false" ] && echo "skipping tests" || eval "$TEST_COMMAND"

We (me) moved the test command into a separate Docker stage, but because the final stage doesn't depend on it, it is pruned from the build and never executes.

If you, for example, force ship to depend on test, it will force the test stage to actually run

FROM test as ship

instead of 

FROM build as ship

I verified that this works.

LucasRoesler commented 8 months ago

I was thinking about this a bit. I am not sure what the best option is here. If the goal is a small final image, running tests is going to work against us, because the env will need all dev time dependencies.

one option that we could use, is to install everything in the build layer. In fact, i woudl install it twice, one generally and then the runtime only dependencies into a virtualenv folder. Then in the final ship layer only needs to copy the virtualenv folder to the final layer and modify the path. I have been using this technique successful in many python images recently and it works very well.

vidz1979 commented 8 months ago

I confirm that doing FROM test as ship works!

ccfontes commented 7 months ago

Running into the similar issue using the FROM build as test tactic to reduce final image size (inspired by this Python template) in faas-bb template.

I find it intriguing we're running into this issue more recently although the apparent root cause is old - Docker multi stage build ignoring intermediate stages, caused by DOCKER_BUILDKIT. Folks in the ticket mention DOCKER_BUILDKIT=0 fixing the issue, but I could see this being a problem to support multiple architectures.

From what I understand @LucasRoesler proposes a python specific sandboxing solution. If we could come up with a more generic solution, folks in other langs for openfaas could benefit too.

ccfontes commented 7 months ago

Just chiming in that DOCKER_BUILDKIT=0 faas build .. did the trick of not ignoring the orphan intermediate stage. Should also work with this template. For now going with this solution. Hope it helps.

LucasRoesler commented 7 months ago

In terms of generic solutions. Obviously the "disable buildkit" works for now, but you can imagine this might eventually be deprecated, I am pretty sure it is already the default builder for most installations now. I hate depending on this or even needing to tell every new user that they have to disable a defaultndocker behavior to get th expected behavior.

For the solution to be fully generic for any language and avoid including the test later in the final image, it means we have to do something via docker itself. The only approach I see is to allow passing a build target to the --target flag so that users can explicitly run the build layer. Each template would need to document how to run tests, if possible.

Every other solution I can think of would be highly language specific.

ccfontes commented 7 months ago

I ended up going for a language specific solution to not ship test related assets, for the same reason I didn't think it was good UX having to pass (and document) DOCKER_BUILDKIT=0. Also as you mentioned, might get deprecated as option. Also unmentioned here that having buildkit enabled has other advantages like running multiple stage builds in parallel from what I read briefly.

I'll have a look into using build target and implications to UX. Thanks.

LucasRoesler commented 7 months ago

@ccfontes if we get some input from @alexellis i would be happy to add a new flag to the faas-cli build or refactor this image so that it runs the tests in a non-orphaned layer.

The python specific alternative that i can think of is to install the whole env in the build layer, run the tests, then install the env again into a venv folder. It is quite easy to move the venv to the final layer and adjust the PATH variable to enable it. This is a bigger change and wouldn't really do much for this specific template/image because we don't support the concept of dev or test dependencies, it is a single requirements.txt, so the build and final layers have the same dependencies regardless. The venv approach would be easier with a tool like Hatch or Poetry or if we documented a flow with requirements-dev.txt (or similar)

alexellis commented 5 months ago

Does it makes sense to go with this solution?

https://github.com/openfaas/python-flask-template/issues/71#issuecomment-1793766261

ccfontes commented 5 months ago

@alexellis If you have an idea why it makes sense or doesn't make sense to go with https://github.com/openfaas/python-flask-template/issues/71#issuecomment-1793766261 solution, it would be helpful to state that idea, would you agree?

alexellis commented 5 months ago

I'm not trying to get too involved here, just suggesting that we go with Lucas' solution. The only one he mentioned in the linked comment.

Screenshot 2024-02-01 at 09 51 10

Do you all agree, or do you want something else?

ccfontes commented 5 months ago

I misread, and very sorry for the misunderstanding. 🙏

I'm OK with @LucasRoesler's solution. Doing the same in faas-bb dockerfile (kind of). It makes a language specific cleanup of the test deps.

LucasRoesler commented 5 months ago

@ccfontes and @alexellis i has pushed a PR https://github.com/openfaas/python-flask-template/pull/73