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

Python function test #71

Open
3 tasks done
vidz1979 opened this issue Oct 28, 2023 · 14 comments · May be fixed by #73
Open
3 tasks done

Python function test #71

vidz1979 opened this issue Oct 28, 2023 · 14 comments · May be fixed by #73

Comments

@vidz1979
Copy link

vidz1979 commented Oct 28, 2023

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

  • FaaS-CLI version ( Full output from: faas-cli version ):

CLI:
commit: f72db8de657001a2967582c535fe8351c259c5f6
version: 0.16.17

  • Docker version docker version (e.g. Docker 17.0.05 ):
    Docker Community 24.0.7

  • Are you using Docker Swarm or Kubernetes (FaaS-netes)?
    Kubernetes

  • Operating System and version (e.g. Linux, Windows, MacOS):
    Linux Ubuntu

  • Code example or link to GitHub repo or gist to reproduce problem:

  • Other diagnostic information / logs from troubleshooting guide

Next steps

You may join Slack for community support.

@alexellis alexellis transferred this issue from openfaas/faas Nov 3, 2023
@alexellis
Copy link
Member

@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
Copy link
Member

@alexellis i just tested this, it is a bug, I can replicate it and the cause was this 79b444b

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
Copy link
Member

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
Copy link
Author

vidz1979 commented Nov 8, 2023

I confirm that doing FROM test as ship works!

@ccfontes
Copy link

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
Copy link

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
Copy link
Member

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
Copy link

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
Copy link
Member

LucasRoesler commented Nov 20, 2023

@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
Copy link
Member

Does it makes sense to go with this solution?

#71 (comment)

@ccfontes
Copy link

@alexellis If you have an idea why it makes sense or doesn't make sense to go with #71 (comment) solution, it would be helpful to state that idea, would you agree?

@alexellis
Copy link
Member

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
Copy link

ccfontes commented Feb 1, 2024

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 LucasRoesler linked a pull request Feb 6, 2024 that will close this issue
12 tasks
@LucasRoesler
Copy link
Member

@ccfontes and @alexellis i has pushed a PR #73

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 a pull request may close this issue.

4 participants