search

openfaas / python-flask-template

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

Use tox to test the python3-flask template #42

Closed LucasRoesler closed 3 years ago

LucasRoesler commented 3 years ago

What

Motivation and Context

This is a PoC based on based on the experience in https://lucasroesler.com/posts/2021/test-python-functions/ and motivated by discussions from our weekly community calls.

How Has This Been Tested?

  1. Using faas-cli new echo0 --lang python3-flask && fass-cli build -f echo0.yml should work in this branch of the repo
    1. now edit the sample unit test to a failure case, for example add assert 1 == 2``, nowfass-cli build -f echo0.yml` will fail
    2. similarly, if you delete the test file, delete all the content of the test file, or delete the content of the tox.ini file, the build will also fail
  2. Using faas-cli new echo1 --lang python3-flask then follow the directions to disable testing in the tox.ini file, then run faas-cli build -f echo1.yml. This should also build successfully

Types of changes

Checklist:

alexellis commented 3 years ago

@viveksyngh please could you take a look and try this out with some of your own tests?

kylos101 commented 3 years ago

Hi @LucasRoesler,

I've never heard of Tox, thank you for sharing. :)

I like the idea: use Tox to run linting and testing for python3-flask functions, and if it fails, fail the faas-cli build.

I would use it too, if it were implemented like this. Here is one of my Python functions, I just test it with Pytest - no Tox. However, I just hammered in Tox, and was able to get it to lint and test, too! If you clone the repo, you can run Tox with: cd twilio_check_verify/ && tox -l == 0 && tox && cd ..

Some thoughts:

  1. A subset of users will find it noisy or prescriptive to see a Tox file or have to disable it (even though its simple, like you mention)
  2. Some users may already be using some other means for test execution, linting, virtual environments
  3. Other language templates will not benefit from this change
  4. This could be breaking (which you also mention)

I am wondering, is there a way to accomplish this workflow, not have it be breaking and be low code like your approach here? Almost like a way for people to "opt into" a workflow such as this one?

One idea it to have faas-cli look in the stack.yaml for hooks (like NPM does with package.json). Here is an example of what I am thinking. I am unsure what would have to change (component wise) for hooks to work. For example, would it just be faas-cli, or would it be all of the templates, too? Let me know if it'd be best to open a related issue in the faas-cli repo instead.

Talk soon,

Kyle

LucasRoesler commented 3 years ago

Hi Kyle, these are all good points. I have thought of many of them as well. It is a difficult balance.

I like the thought about having hooks. Although, in some ways this is exactly what tox is doing because it provides an entrypoint in which a user can define any commands and a default sequence of commands to run when we call tox without any arguments. In this way it might be more powerful than a hook system in the CLI. Originally we had discussed just picking one of the test frameworks and I was going to pick Pytest but after researching tox more (i had originally thought it just another framework like Nose) I thought this would be the most flexible while still picking an existing tool from the Python ecosystem and still being very pythonic.

RE point (3), there are already several templates that do have a similar pattern of running tests during the faas-cli build. The Go templates are one example. Given the popularity (that i have seen) of Go, Node, and Python functions in the OpenFaaS community, It seems nice/good to have testing in these templates as a "first class citizen" and hopefully reducing some of the friction.

I think we can handle the backwards compatibility by adding an additional checks prior to running tox, maybe wrap it in a small bash script to help make the check more robust: either tox.ini file exists or the pyproject file contains a tox section. This might also reduce concerns about noise by letting people just delete the tox file. How would you feel if it was easy enough to just disable by deleting the tox file?

Ultimately though, the templates are very easy to fork and replace, especially when people use this https://docs.openfaas.com/reference/yaml/#yaml-template-stack-configuration or commit the template folder and modify the template as works best for them.

LucasRoesler commented 3 years ago

@alexellis and @kylos101 what do you think of doing something like this 

ARG TEST_COMMAND=tox
RUN if [ "x$TEST_COMMAND" = "xoff" ]; then \
    echo "skipping tests";\
    else \
    eval "$TEST_COMMAND"; \
    fi

This allows you to do 

 faas-cli build -f echo0.yml --build-arg TEST_COMMAND='off'

you can also add it as a build arg in the stack.yml via https://docs.openfaas.com/reference/yaml/#function-build-args-build-args

This gives the DX that the developer can add the build arg to the stack.yml and then delete the tox.ini file . They can even add their own custom script, if they want, providing maximum flexibility. At the same time, the default experience still includes and encourages testing.

A note about the script. I know that this looks funny "x$TEST_COMMAND" = "xoff" the reason for doing this is that if you just use $TEST_COMMAND = "off", this becomes tox = "off" and it actually evaluates the tox command, which is not desirable, it will run the comamnd twice once inside the brackets if [ ..] and then again inside the else block. By prefixing with and x is avoids the chance that the strings looks like a valid command and the if [...] becomes a simple string comparison.

kylos101 commented 3 years ago

Hi @LucasRoesler ,

That is really cool! I'm sorry about the delay. Seriously though, good use of an arg.

I'm remote right now, but will poke around later tonight. To see what it's like, and if I can help add any value.

kylos101 commented 3 years ago

Hi @LucasRoesler,

I went through the test notes, thank you for sharing such detailed steps! As an occasional contributor, that helped me quickly dig in and start providing value right away.

Additionally, I appreciate your thoughtful response to my initial comment, letting me know that other templates have built-in test functionality too. For example, that tipped me of that I could look at their respective Dockerfile's, to see how they are linting and testing.

Thoughts on next steps:

  1. Consider deleting ./template/handler_test.py to be consistent with Go and Node function templates. This is based on Observations 3, 5, and 6 below.
  2. What is the expected behavior for faas-cli build -f echo0.yml if a template file like ./echo0/handler_test.py gets modified in the function folder? Currently it leaves the modified file in the function folder and builder folder, which seems correct to me.
  3. What is the expected behavior for faas-cli build -f echo0.yml if a template file like ./echo0/handler_test.py is deleted from the function folder? Currently it is not added back to the function folder, and is added to the build folder, like we saw with Observation 3 (below). Adding a deleted file back to the build folder seems incorrect and hides that I broke a function (it will no longer build correctly with the underlying template). I could be being naive here, perhaps it is good that missing files are regenerated. Should I open a separate issue for faas-cli to focus that conversation?
  4. Should the linter return a non-zero exit code if there is a non-test Python file is empty? Currently it does not, and the build succeeds. I could not find a flake8 rule to cause a linter error for this scenario (I tested E301, and W391).
  5. I like TEST_COMMAND as the arg name, if we change Tox with something else in the future, we can still use this command.

Observations:

  1. The initial build example command had a slight typo fass-cli build -f echo0.yml, but was a quick to solve, faas-cli build -f echo0.yml
  2. I was able to see the linter and tests run for a newly built function following your PR instructions.
  3. When I delete handler_test.yml from my function like so: rm ./echo0/handler_test.py, and then build with faas-cli build -f echo0.yml, the build passes when I expected it to fail. I think this is because the ./template/python3-flask/function/handler_test.py is being copied to ./build/echo/function/handler_test.py - effectively bypassing my function function folder (even though I deleted ./echo0/handler_test.py).
  4. When I build using an empty ./echo0/handler_test.py, the build fails as expected.
  5. For Node14 template, I noticed (1) we run tests with npm test and (2) there are no out of the box tests to copy into the function folder.
  6. For golang-http, I noticed (1) we run tests with go test handler/function... -cover and (2) there are no out of the box tests to copy into the function folder.
  7. When I provide my own empty test file ./echo0/service_test.py, the build fails as expected with 
Errors received during build:
- [echo0] received non-zero exit code from build, error: The command '/bin/sh -c if [ "x$TEST_COMMAND" = "xoff" ]; then     echo "skipping tests";    else     eval "$TEST_COMMAND";     fi' returned a non-zero code: 1
  1. An empty non-test file like ./echo/service.py does not cause the build to fail.
  2. A file with invalid syntax like the below causes the linter to return a non-zero exit status and the build to fail, which is good.

I've got to step away now, but will try to test some more later. Take care, bud!

kylos101 commented 3 years ago

Some additional tests I ran:

LucasRoesler commented 3 years ago

@kylos101 RE

  1. Consider deleting ./template/handler_test.py to be consistent with Go and Node function templates. This is based on Observations 3, 5, and 6 below.

The file needs to exist if we decide testing is on by default and using pytest (via tox), then pytest will fail if it finds no test cases to run, see https://github.com/pytest-dev/pytest/issues/812 and https://github.com/pytest-dev/pytest/issues/662

pytest has chosen that that not finding any tests should be considered a failure because it most likely indicates a configuration error and it is better to error early and loud instead of silently missing that no tests were run. If we do not include at least one test and we run pytest by default, then it will always fail.

RE

  1. What is the expected behavior for faas-cli build -f echo0.yml if a template file like ./echo0/handler_test.py gets modified in the function folder? Currently it leaves the modified file in the function folder and builder folder, which seems correct to me.
  2. What is the expected behavior for faas-cli build -f echo0.yml if a template file like ./echo0/handler_test.py is deleted from the function folder? Currently it is not added back to the function folder, and is added to the build folder, like we saw with Observation 3 (below). Adding a deleted file back to the build folder seems incorrect and hides that I broke a function (it will no longer build correctly with the underlying template). I could be being naive here, perhaps it is good that missing files are regenerated. Should I open a separate issue for faas-cli to focus that conversation?

For (2), yes, the modified file replaced the file from the template, it may not be obvious, but this is exactly the same behavior as what already happens to the handler.py file.

For (3), it is also expected that the default template file is copied. In this case, the file/test is completely self-contained no-op. It should never cause a build failure because the function literally does nothing. But the existence also means that pytest will not fail (see the response to 1).

RE

  1. Should the linter return a non-zero exit code if there is a non-test Python file is empty? Currently it does not, and the build succeeds. I could not find a flake8 rule to cause a linter error for this scenario (I tested E301, and W391).

The failure for you empty test file is actually a failure due to pytest, again see my response to (1).

Additionally, the linter configuration is inspired from https://docs.github.com/en/actions/guides/building-and-testing-python and is not intended to enforce all of the flake8/pep8 rules, rather, the flake8 rules chosen as the default highlight actual syntax errors that we would expect to cause runtime errors. This feels appropriate as a test because the function is broken.

It did not feel appropriate to add too much here but it did seem like it would be useful to catch and fail on a syntax error. The goal was to make it easy for users to configure it with more rules if necessary via https://github.com/openfaas/python-flask-template/pull/42/files#diff-cab7f7374a89adeb5b48fe4d2cb45bc59ec7534a4207c4dcd2771fc1712352ebR34-R41

The tooling and linting rules in the python community are not exactly universal yet. But, having it there makes it clear (i hope) for how to integrate your favorite tools and settings into the build flow.

Reviewing my comment above, it occurs to me that almost everything seems to tie back to "pytest fails if there are no tests"

kylos101 commented 3 years ago

Perfect, your reply handles my thoughts Re: next steps, @LucasRoesler. :+1:

For what it's worth, I like the idea of it being on by default. If someone finds this to be breaking, they can update their stack file or CI scripting to use build arg.

Also, I didn't realize you could reference multiple lines of code in the diff - super helpful! Thank you.

Let me know if there's anything else I can do to help?

LucasRoesler commented 3 years ago

At this point, i think the next step is to get a design approval from @alexellis

I think one thing to add is that this pattern could easily be copied to any of the other templates and could be very productive, even in the templates that already have test support, e.g. Go, because the user may want to customize the flags given to the test command, for example so that only unit tests are run and integration tests are skipped