Skip to content
This repository has been archived by the owner on Feb 1, 2023. It is now read-only.

[WIP] Use tox and npm scripts #272

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

vitorbaptista
Copy link
Contributor

This PR:

  • Adds tox for testing/linting Python
  • Moves the JS-related scripts (running tests, building, etc.) to package.json (from the Makefile)

There's not much advantage in using tox instead of the Makefiles, considering we just have a single Python version. I did it because I find it useful to understand the testing configuration, and I think it makes it easier for newcomers to just run the tests. With tox, they just have to:

cp .env.example .env
# Modify the TEST_DATABASE_URL with the test DB to use
tox

I also changed all example values in .env.example to be valid (and commented out SENTRY_DSN, otherwise it'll be used even with an invalid value), to make as easy as possible for new users.

Then I moved the NodeJS scripts from the Makefile to the package.json. This is important to guarantee that we're using the binaries that were installed by npm, not something that we happen to have in our path. By doing this, I fixed a tricky bug. We were using the binaries installed by npm by modifying the PATH in the Makefile, as:

export PATH := $(PATH):./node_modules/.bin

However, this appends ./node_modules/.bin to the end of the PATH variable. Binaries are searched in the folders inside PATH in the order that they appear, so if (say) you had the karma binary installed in your system, it would be used instead of the one installed in ./node_modules.

Copy link
Member

@amercader amercader left a comment

Choose a reason for hiding this comment

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

@vitorbaptista tox is fine (seems overkill for just a python version but you are the expert in bootstrapping python packages :) ).

Regarding scripts in package.json, we used to have them there and then moved them back to Makefile to have a single point of truth. I personally don't mind your new approach, so unless @roll has any comments I'm happy to merge this.

@amercader
Copy link
Member

If tests pass of course

@roll
Copy link
Member

roll commented Oct 6, 2017

I'm +1 to get back to package.json scripts. The reason of moving things to Makefile was to find some structure in overwhelming amount of command we had to write at some point. But Makefile is not really helping in it as it's been turned out.

PS.
As we was discussing with Adria I'm not sure why things like setup.py or tox.ini should exist in web apps at all. In my eyes a web app is just one environment directly interpreted set of scripts. So) But I don't mind any)

@vitorbaptista
Copy link
Contributor Author

Great! I'll check why the tests aren't passing and update the PR.

As we was discussing with Adria I'm not sure why things like setup.py or tox.ini should exist in web apps at all. In my eyes a web app is just one environment directly interpreted set of scripts. So) But I don't mind any)

I mostly agree regarding setup.py. Unless we're publishing to pypi as part of our deployment process, it doesn't bring us much. However, I think every Python project should use tox (or a similar tool).

It's much easier (for a newcomer) to run the tests, being sure the environment is correctly set. It's also useful to run linting. It can even be (ab)used to write something similar to npm scripts, where a command is executed with all the dependencies installed, so you could have tox -e start to start the project, removing the need for a virtualenv entirely.

It's even more useful in our case, where we have many different Python repositories, so we can be sure that wherever we run tox, it's using the correct dependencies and Python versions. There are other ways to do it (like configuring your shell to automatically enable the virtualenv), but tox is easier IMHO.

It's required to run `setup.py`, so we need to explicitly add it to the
`MANIFEST.in`, otherwise it won't be added to the python package.
@vitorbaptista vitorbaptista force-pushed the use-tox-and-npm-scripts branch 5 times, most recently from dae00e8 to 2f04764 Compare January 4, 2018 17:10
The Makefile then is simply a dumb wrapper that calls commands defined in the
`package.json` or `tox.ini`, depending if it's JavaScript or Python. This
allows us to rely on their respective package manager to run the correct
version of each executable (avoiding having to mess with $PATH).

I also changed the testing NODE_ENV to "test", as it's the most common name.
This fixes a bug where psycopg2 is unable to determine postgresql versions
>10.x (see psycopg/psycopg2#489).
The fixes were mainly adding missing `__init__.py` files to folders that
should've been modules but weren't. I also changed to use `pkg_resources` to
find the resource files path, as it's more reliable than relying on the
__file__ location.
@roll roll added the wip label Apr 15, 2019
@roll roll changed the title Use tox and npm scripts [WIP] Use tox and npm scripts Nov 7, 2019
@roll roll added enhancement and removed wip labels Nov 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants