Use unittest for tests instead of trial

[babolivier]

When running tox -e py locally to run tests I realised it complained that it couldn't find setup.py (which makes sense since we switched to setup.cfg) unless I ran rm -r .tox/ before every run. When digging, I realised it's because we run tox with usedevelop=true, which, according to docs, relies on setup.py to work.

We only do this because we want to run trial in CI, but imho the burden of having to recreate the tox environment for every run outweighs the advantages of using trial instead of e.g. Python's own unittest module. So my suggestion is that we stop using trial in favour of unittest.

[callahad]

When digging, I realised it's because we run tox with usedevelop=true, which, according to docs, relies on setup.py to work.

I'm a little skeptical of this diagnosis, insofar as tox's default install_command is python -m pip install {opts} {packages} and usedevelop literally just includes -e in that command line.

So it should never actually attempt to run setup.py develop. Is your pip too old to support editable installs with just setup.cfg?

[callahad]

(You need at least pip 21.1, from 2021-04-24, for editable installs with just setup.cfg to work)

[babolivier]

Hmm, that's weird. Here's the output of trying to run tox -e py twice on the EMS module I've been working on lately:

$ pip --version
pip 21.3.1 from /home/babolivier/.virtualenvs/synapse-py39/lib/python3.9/site-packages/pip (python 3.9)
$ tox -e py
py create: /home/babolivier/Documents/matrix/ems-synapse-background-update-controller/.tox/py
py installdeps: git+https://github.com/matrix-org/synapse.git@develop#egg=matrix-synapse
py develop-inst: /home/babolivier/Documents/matrix/ems-synapse-background-update-controller
py installed: aiounittest==1.4.1,attrs==21.2.0,Automat==20.2.0,backports.entry-points-selectable==1.1.1,bcrypt==3.2.0,black==21.9b0,bleach==4.1.0,boto3==1.20.17,boto3-stubs==1.20.17,botocore==1.23.17,botocore-stubs==1.23.17,canonicaljson==1.5.0,certifi==2021.10.8,cffi==1.15.0,charset-normalizer==2.0.8,click==8.0.3,constantly==15.1.0,cryptography==36.0.0,distlib==0.3.3,-e git+https://gitlab.matrix.org/new-vector/ems-synapse-background-update-controller.git@832a4895ede04ec4edc39fc94a9bd43e6a8007a3#egg=ems_background_update_controller,filelock==3.4.0,flake8==4.0.1,frozendict==2.1.1,hyperlink==21.0.0,idna==3.3,ijson==3.1.4,importlib-metadata==4.8.2,incremental==21.3.0,isort==5.9.3,Jinja2==3.0.3,jmespath==0.10.0,jsonschema==4.2.1,MarkupSafe==2.0.1,matrix-synapse @ git+https://github.com/matrix-org/synapse.git@153194c7717d8016b0eb974c81b1baee7dc1917d,mccabe==0.6.1,msgpack==1.0.3,mypy==0.910,mypy-boto3-cloudwatch==1.20.9,mypy-extensions==0.4.3,netaddr==0.8.0,packaging==21.3,pathspec==0.9.0,phonenumbers==8.12.38,Pillow==8.4.0,platformdirs==2.4.0,pluggy==1.0.0,prometheus-client==0.12.0,py==1.11.0,pyasn1==0.4.8,pyasn1-modules==0.2.8,pycodestyle==2.8.0,pycparser==2.21,pyflakes==2.4.0,pymacaroons==0.13.0,PyNaCl==1.4.0,pyOpenSSL==21.0.0,pyparsing==3.0.6,pyrsistent==0.18.0,python-dateutil==2.8.2,PyYAML==6.0,regex==2021.11.10,requests==2.26.0,s3transfer==0.5.0,service-identity==21.1.0,signedjson==1.1.1,simplejson==3.17.6,six==1.16.0,sortedcontainers==2.4.0,toml==0.10.2,tomli==1.2.2,tox==3.24.4,treq==21.5.0,Twisted==21.7.0,typing_extensions==4.0.1,unpaddedbase64==2.1.0,urllib3==1.26.7,virtualenv==20.10.0,webencodings==0.5.1,wrapt==1.13.3,zipp==3.6.0,zope.interface==5.4.0
py run-test-pre: PYTHONHASHSEED='806511979'
py run-test: commands[0] | python -m unittest discover
....
----------------------------------------------------------------------
Ran 4 tests in 0.009s

OK
___________________________________________________________________________________________________________________ summary ___________________________________________________________________________________________________________________
  py: commands succeeded
  congratulations :)
$ tox -e py
/home/babolivier/Documents/matrix/ems-synapse-background-update-controller/.tox/py/bin/python: can't open file '/home/babolivier/Documents/matrix/ems-synapse-background-update-controller/setup.py': [Errno 2] No such file or directory
ERROR: invocation failed (exit code 2)
___________________________________________________________________________________________________________________ summary ___________________________________________________________________________________________________________________
ERROR:   py: InvocationError for command /home/babolivier/Documents/matrix/ems-synapse-background-update-controller/.tox/py/bin/python /home/babolivier/Documents/matrix/ems-synapse-background-update-controller/setup.py --name (exited with code 2)

(edit: note that this shows a pip version in my virtualenv, which tox might or might not pick up, but my system pip is running the same version)

[reivilibre]

I also am skeptical because I don't recall having the issue you're facing, and moreover trial might be necessary for running tests that actually make use of Twisted's reactors, as far as I understand?

[reivilibre]

Hm actually I can reproduce now, not sure why I haven't ran into it before..

❯ tox -e py -vv
using tox.ini: /home/rei/work/PS/synapse-user-restrictions/tox.ini (pid 174350)
  removing /home/rei/work/PS/synapse-user-restrictions/.tox/log
using tox-3.24.4 from /home/rei/work/PS/synapse-user-restrictions/.venv/lib/python3.9/site-packages/tox/__init__.py (pid 174350)
skipping sdist step
/home/rei/work/PS/synapse-user-restrictions/.venv/bin/python3.9 (/home/rei/work/PS/synapse-user-restrictions/.venv/bin/python3.9) is {'executable': '/home/rei/work/PS/synapse-user-restrictions/.venv/bin/python3.9', 'implementation': 'CPython', 'version_info': [3, 9, 7, 'final', 0], 'version': '3.9.7 (default, Sep 10 2021, 14:59:43) \n[GCC 11.2.0]', 'is_64': True, 'sysplatform': 'linux', 'os_sep': '/', 'extra_version_info': None}
py uses /home/rei/work/PS/synapse-user-restrictions/.venv/bin/python3.9
py start: getenv /home/rei/work/PS/synapse-user-restrictions/.tox/py
py reusing: /home/rei/work/PS/synapse-user-restrictions/.tox/py
py finish: getenv /home/rei/work/PS/synapse-user-restrictions/.tox/py after 0.03 seconds
py start: developpkg /home/rei/work/PS/synapse-user-restrictions
[174356] /home/rei/work/PS/synapse-user-restrictions$ /home/rei/work/PS/synapse-user-restrictions/.tox/py/bin/python /home/rei/work/PS/synapse-user-restrictions/setup.py --name
/home/rei/work/PS/synapse-user-restrictions/.tox/py/bin/python: can't open file '/home/rei/work/PS/synapse-user-restrictions/setup.py': [Errno 2] No such file or directory
ERROR: invocation failed (exit code 2)
py finish: developpkg /home/rei/work/PS/synapse-user-restrictions after 0.02 seconds
___________________________________________________________________________________________________ summary ____________________________________________________________________________________________________
ERROR:   py: InvocationError for command /home/rei/work/PS/synapse-user-restrictions/.tox/py/bin/python /home/rei/work/PS/synapse-user-restrictions/setup.py --name (exited with code 2)

My (limited) understanding (and what happened experimentally) is that removing use_develop will mean that tox has to sdist your project and reinstall it every time it changes, which doesn't sound optimal either.

I also disagree with your interpretation of

When digging, I realised it's because we run tox with usedevelop=true, which, according to docs, relies on setup.py to work.

The docs says (to my reading) it will use the -e flag and performs an install in the style of setup.py develop. I also notice that the failing command is setup.py --name; not setup.py develop anyway :/.

As to why it's trying to do setup.py --name, it looks like it's a check to see if the package needs reinstalling: https://github.com/tox-dev/tox/blob/51cc218e71949e90ab7982bbb12a475fcbb90613/src/tox/venv.py#L326-L329 . :/ Oddly, they construct the path to setup.cfg as though they were prepared to deal with that case, but then just use setup.py anyway..?

The simplest naïve workaround might be to have the 2-line stub setup.py, but what a shame after we've worked so hard to make that unnecessary so far.

I notice that someone on the tox repo seems to have found a workaround which we could use for now: tox-dev/tox#2197 (comment)

(in essence, add --editable . to the deps instead of usedevelop)

Here's an example diff doing just that: https://github.com/ansible-community/ansible-compat/pull/96/files

I reckon we can either do that or wait for this PR: tox-dev/tox#2273

[callahad]

My (limited) understanding (and what happened experimentally) is that removing use_develop will mean that tox has to sdist your project and reinstall it every time it changes, which doesn't sound optimal either.

Removing use_develop is more work, but arguably more correct. Hynek has a blog post on Testing & Packaging which says:

Less than a year later, I received a bug report that my tox and coverage setup works by accident: my tests didn’t run against the version of my app that got installed into tox’s virtual environments. They ran against the actual directory. Let me reinforce this point once more:

Your tests do not run against the package as it will be installed by its users. They run against whatever the situation in your project directory is.

But this is not just about tests: the same is true for your application. The behavior can change completely once you package and install it somewhere else.

All of this makes you miss packaging issues which are especially frustrating to track down: ever forgot to include a resource (like templates) or a sub-package? Ever uploaded an empty package to PyPI?

So let's try to remove the reliance on editable install for Tox / CI?

[callahad]

Ionel Mărieș has a nice blog post that delves deeper into package structure and testing in its first two sections. Everything from "The Setup Script" and beyond isn't particularly relevant to this, but the earlier sections are good, even when separated from a discussion of src/ layouts or not.

Amusingly, he ends with:

Most projects use them incorectly, as all the test runners except Twisted's trial have incorrect defaults for the current working directory - you're going to test the wrong code if you don't test the installed code. trial does the right thing by changing the working directory to something temporary, but most projects don't use trial.

[babolivier]

I'm going to close this PR on the basis that we've identified an actual issue but I misidentified its source and this patch isn't the right fix for it.

[babolivier]

The docs says (to my reading) it will use the -e flag and performs an install in the style of setup.py develop

Also, as a nitpick, maybe that's an issue with me not being a native english reader, but that sentence to me means that it uses setup.py develop to install the package, and pip -e comes into the mix at some point in a blurry and unspecified way. I'll open an issue on tox's repo about this.

[callahad]

It's genuinely ambiguous in Tox's docs; I had to dive into the source to determine what it meant.

[anoadragon453]

"in the style of" usually means that it "does something similar to", rather than explicitly using it.

For example, I might do a code review in the style of richvdh, which means that I may use techniques I've noticed he's used. Rather than getting out my richvdh to do the review.

[babolivier]

"in the style of" usually means that it "does something similar to", rather than explicitly using it.

Except the doc says

Install the current package in development mode with “setup.py develop”

I would have understood "in the style of", but this to me means the package is installed with setup.py.

Also, sorry, I realise now that while by "that sentence" I was talking about the sentence from the docs it could also be understood as the sentence from the quote. I thought I added enough context to my comment to make it clear what I was referring to, but apparently didn't.

Anyway, I've opened an issue on their side: tox-dev/tox#2282