Add tox replacement #1558

Kyle-Verhoog · 2020-07-07T07:04:57Z

Tox is slow and the configuration language is unwieldy with our use case.

This PR introduces a custom solution that aims to provide a better specified and more optimized solution for running our matrix of tests.

Preliminary results (without pre-generating and sharing base virtual envs, which could save about 2m in the best case):

tracer: 33m 8s -> 6m 41s (79.8% reduction)
django: 45m 34s -> 18m 38s (59.1% reduction)
redis 22m 9s -> 4m 55s (77.8% reduction)

Configuration Example (Django)

    Suite(
        name="django",
        command="pytest tests/contrib/django",
        env=[("TEST_DATADOG_DJANGO_MIGRATION", [None, "1"])],
        cases=[
            Case(
                pys=[2.7, 3.5, 3.6],
                pkgs=[
                    ("django-pylibmc", [">=0.6,<0.7"]),
                    ("django-redis", [">=4.5,<4.6"]),
                    ("pylibmc", [""]),
                    ("python-memcached", [""]),
                    ("django", [">=1.8,<1.9", ">=1.11,<1.12"]),
                ],
            ),
            Case(
                pys=[3.5],
                pkgs=[
                    ("django-pylibmc", [">=0.6,<0.7"]),
                    ("django-redis", [">=4.5,<4.6"]),
                    ("pylibmc", [""]),
                    ("python-memcached", [""]),
                    ("django", [">=2.0,<2.1", ">=2.1,<2.2"]),
                ],
            ),
            Case(
                pys=[3.6, 3.7, 3.8],
                pkgs=[
                    ("django-pylibmc", [">=0.6,<0.7"]),
                    ("django-redis", [">=4.5,<4.6"]),
                    ("pylibmc", [""]),
                    ("python-memcached", [""]),
                    ("django", [">=2.0,<2.1", ">=2.1,<2.2", ">=2.2,<2.3", ">=3.0,<3.1", ""]),
                ],
            ),
        ],
    ),

Equivalent in tox:

[tox]
envlist =
    django_contrib{,_migration}-py{27,35,36}-django{18,111}-djangopylibmc06-djangoredis45-pylibmc-redis{210}-memcached
    django_contrib{,_migration}-py35-django{20,21,22}-djangopylibmc06-djangoredis45-pylibmc-redis{210}-memcached
    django_contrib{,_migration}-py{36,37,38}-django{20,21,22,30,}-djangopylibmc06-djangoredis45-pylibmc-redis{210}-memcached

# ...

[testenv]
setenv =
    django_contrib_migration: TEST_DATADOG_DJANGO_MIGRATION=1
# ...
deps=
    django: django
    django18: django>=1.8,<1.9
    django111: django>=1.11,<1.12
    django20: django>=2.0,<2.1
    django21: django>=2.1,<2.2
    django22: django>=2.2,<2.3
    django30: django>=3.0,<3.1
    djangopylibmc06: django-pylibmc>=0.6,<0.7
    djangoredis45: django-redis>=4.5,<4.6
    memcached: python-memcached
    pylibmc: pylibmc

# ...

commands =
    django_contrib: pytest {posargs} tests/contrib/django
    django_contrib_migration: pytest {posargs} tests/contrib/django

Configuration Example (Profiling)

Configuration is done in Python.

    Suite(
        name="profiling",
        command="python -m tests.profiling.run pytest --capture=no --verbose tests/profiling/",
        env=[("DD_PROFILE_TEST_GEVENT", lambda case: "1" if "gevent" in case.pkgs else None),],
        cases=[
            Case(
                 pys=[2.7, 3.5, 3.6, 3.7, 3.8],
                 pkgs=[("gevent", [None, ""])],
            ),
            # Min reqs tests
            Case(
                 pys=[2.7],
                 pkgs=[("gevent", ["==1.1.0"]), ("protobuf", ["==3.0.0"]), ("tenacity", ["==5.0.1"]),]
            ),
            Case(
                pys=[3.5, 3.6, 3.7, 3.8],
                pkgs=[("gevent", ["==1.4.0"]), ("protobuf", ["==3.0.0"]), ("tenacity", ["==5.0.1"]),]
            ),
        ],
    ),

equivalent tox configuration:

[tox]
envlist =
    {py27,py35,py36,py37,py38}-profile{,-gevent}
    {py27,py35,py36,py37,py38}-profile-minreqs{,-gevent}

# ...

[testenv]
setenv =
    profile-gevent: DD_PROFILE_TEST_GEVENT=1

# ...

deps=
    profile-minreqs: protobuf==3.0.0
    profile-minreqs: tenacity==5.0.1
    profile-!minreqs-gevent: gevent
    py27-profile-minreqs-gevent: gevent==1.1.0
    !py27-profile-minreqs-gevent: gevent==1.4.0

# ...

commands =
      profile: python -m tests.profiling.run pytest --capture=no --verbose {posargs} tests/profiling

- "group" -> "suite" - suites have cases - add django, tracer and profiling suites - add some tests - bunch of other small stuff

brettlangdon

from a high level, I like it.

Like we discussed before, this style syntax feels more verbose looking than the normal tox redis_contrib_py{27,35,36,37,38}-redis{21,20,32,34,35} env definition but think it is much more clear what is going on, less jumping around a massive file.

I think any other nit-picks I had (not specified) will be solved when this is it's own module/package.

.circleci/config.yml

riot.py

brettlangdon · 2020-07-07T11:33:52Z

riot.py

+                pkgs=[
+                    ("django-pylibmc", [">=0.6,<0.7"]),
+                    ("django-redis", [">=4.5,<4.6"]),
+                    ("pylibmc", [""]),


I wonder if we should alias "latest" -> "" would it make it more clear to read?

or just latest = "" -> ("pylibmc", [latest]). Either way I think we're in agreement that "" is kinda subtle.

brettlangdon · 2020-07-07T11:48:08Z

riot.py

+    """Return the command string used to execute `cmd` in virtual env located
+    at `venv_path`.
+    """
+    return "source %s/bin/activate && %s" % (venv_path, cmd)


you probably don't have to call source. If you read through activate majority of it is setting up deactivate, and then setting a few environment variables:

export VIRTUAL_ENV=<path_to_venv> export PATH=<venv>/bin:$PATH unset PYTHONHOME

And that is in.

In fact, you can generally get by just calling from <venv>/bin/<cmd> directly without modifying any env variables and it'll work as expected.

Calling source .../bin/activate is probably the right thing to do

riot.py

jd

Ok so I'm a bit surprised by this but it looks definitely good.
I think it needs a few more iteration and polish, but this is a really good starting point to do something cool.

We might want to polish the architecture a bit, using a few classes here and there — and maybe some docstrings.

jd · 2020-07-07T14:02:33Z

.circleci/config.yml

+        default: ""
+    steps:
+      - checkout
+      - run: pip3 install virtualenv  # TODO: this can be baked into the image


Though the image is not updated that often, so it might be better to have the latest virtualenv deployed each time.

jd · 2020-07-07T14:02:58Z

.circleci/config.yml

-      - run_tox_scenario:
-          pattern: '^py..-profile'
+      - run_test:
+          pattern: "profiling"


detail, but you don't need the "" I think

jd · 2020-07-07T14:04:35Z

riot.py

+
+
+# It's easier to read `Suite`, `Case` rather than AttrDict or dict.
+CaseInstance = Case = Suite = AttrDict


Having custom classes such as:

class Case(AttrDict): pass

Is going to be a little bit better when introspecting or debugging.

yepyepyep definitely, a lot of the functions laying around are really "methods" as well. This is purely just a result of iterating a ton and not wanting to deal with class boilerplate 🙂

jd · 2020-07-07T14:08:49Z

riot.py

+
+    ref: https://www.python.org/dev/peps/pep-0508/
+    """
+    return "%s%s" % (libname, version)


Suggested change

return "%s%s" % (libname, version)

return f"{libname}{version}"

;)

jd · 2020-07-07T14:09:11Z

riot.py

+def get_env_str(envs: t.List[t.Tuple]):
+    return " ".join("%s=%s" % (k, v) for k, v in envs)


Suggested change

def get_env_str(envs: t.List[t.Tuple]):

return " ".join("%s=%s" % (k, v) for k, v in envs)

def get_env_str(envs: t.List[t.Tuple]):

return " ".join(f"{k}={v}" for k, v in envs)

jd · 2020-07-07T14:12:50Z

riot.py

+            yield case, env_cfg, py, pkg_cfg
+
+
+def suites_iter(suites, pattern, py=None):


I feel like that this could be a little more organized by using classes.

jd · 2020-07-07T14:13:45Z

riot.py

+            # Copy the base venv to use for this case.
+            logger.info("Copying base virtualenv '%s' into case virtual env '%s'.", base_venv, venv)
+            try:
+                run_cmd(["cp", "-r", base_venv, venv], stdout=subprocess.PIPE)


I'd use shutil.copytree instead?

Originally had it but it won't allow you to overwrite a directory if it already exists.

jd · 2020-07-07T14:14:34Z

riot.py

+        s = "%s %s: %s python%s %s" % (status_char, r.case.suite.name, env_str, r.case.py, r.pkgstr)
+        print(s, file=out)
+
+    if any(True for r in results if r.code != 0):


Suggested change

if any(True for r in results if r.code != 0):

if any(r.code != 0 for r in results):

riot.py

Kyle-Verhoog · 2020-11-11T02:49:20Z

going to close now since @majorgreys and I have got most of this functionality merged in other PRs now. #1737 #1771 #1747 #1786 #1783 and more

Kyle-Verhoog added 10 commits July 3, 2020 00:57

add basic script

f155047

add initial script

902e799

stream results to stdout and more

ea00a9d

add regex matching

b40d763

add error handling

c7682a1

add support for none versions

09eee3a

use riot for redis

d22e297

refactor

1e2115c

- "group" -> "suite" - suites have cases - add django, tracer and profiling suites - add some tests - bunch of other small stuff

use riot for django, profiling and tracer

135a18a

quick self review

dbf5aa4

Kyle-Verhoog added dev/testing labels Jul 7, 2020

brettlangdon reviewed Jul 7, 2020

View reviewed changes

jd reviewed Jul 7, 2020

View reviewed changes

Kyle-Verhoog added 2 commits July 7, 2020 14:49

wip

3369a48

add vendored library

c006b0f

Kyle-Verhoog force-pushed the riot branch from 56af644 to c006b0f Compare July 16, 2020 14:30

Merge branch 'master' into riot

83fff87

Kyle-Verhoog force-pushed the riot branch 3 times, most recently from 242c390 to 43e6f58 Compare July 16, 2020 16:48

generate base venvs and persist/restore from workspace

c006395

Kyle-Verhoog force-pushed the riot branch from 43e6f58 to c006395 Compare July 16, 2020 17:01

majorgreys and others added 6 commits July 16, 2020 14:10

add flask, flask_cache, internal

7c90cf9

fix install

79c1d92

Merge branch 'riot' of github.com:DataDog/dd-trace-py into riot

2a87c58

build c extensions as a part of the install

cf23796

add opentracer

5a3bcad

add flask_autopatch

d1915e1

Kyle-Verhoog added 4 commits July 17, 2020 02:57

move tracer tests to tests/tracer

df05b8c

move opentracing tests to opentracing/core

96e9762

fix imports

16b572c

get_dummy_tracer -> DummyTracer()

95c7212

Kyle-Verhoog force-pushed the riot branch from eb19806 to bae6a77 Compare July 17, 2020 08:19

move all test helpers to tests/base.py

2f0abf8

Kyle-Verhoog force-pushed the riot branch from bae6a77 to 2f0abf8 Compare July 17, 2020 08:31

Kyle-Verhoog added 8 commits July 17, 2020 11:07

move propagation tests

7242561

move more utils to tests/base; delete unused memory.py

346fc64

add httplib

38c8911

add sqlite, vertica

062e51f

fix django, requests imports

389c0bb

add dbapi

e398e6a

add cassandra

0881db4

add rediscluster

0df8676

Kyle-Verhoog force-pushed the riot branch from f77fb65 to 0df8676 Compare July 17, 2020 16:15

Kyle-Verhoog added 5 commits July 17, 2020 12:26

various fixes

8bd9d4e

add grpc

7c3e09d

remove unnecessary riot files

9afae02

move test modules to submodule

9e5b27c

fix flaskcache import

6ca5ca5

Kyle-Verhoog force-pushed the riot branch from dd384b9 to 4a2d103 Compare July 17, 2020 22:30

add instrumentation

6a84e87

Kyle-Verhoog force-pushed the riot branch from 4a2d103 to 6a84e87 Compare July 17, 2020 22:38

update riot

0e64dbc

Kyle-Verhoog force-pushed the riot branch from 659908d to 0e64dbc Compare July 17, 2020 23:24

Kyle-Verhoog mentioned this pull request Aug 3, 2020

tests: organize test helpers #1589

Merged

Kyle-Verhoog closed this Nov 11, 2020

Kyle-Verhoog deleted the riot branch November 11, 2020 02:49



		# It's easier to read `Suite`, `Case` rather than AttrDict or dict.
		CaseInstance = Case = Suite = AttrDict

	return "%s%s" % (libname, version)
	return f"{libname}{version}"

		def get_env_str(envs: t.List[t.Tuple]):
		return " ".join("%s=%s" % (k, v) for k, v in envs)

		yield case, env_cfg, py, pkg_cfg


		def suites_iter(suites, pattern, py=None):

	if any(True for r in results if r.code != 0):
	if any(r.code != 0 for r in results):

Add tox replacement #1558

Add tox replacement #1558

Uh oh!

Conversation

Kyle-Verhoog commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Configuration Example (Django)

Configuration Example (Profiling)

Uh oh!

brettlangdon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kyle-Verhoog commented Nov 11, 2020

Uh oh!

Uh oh!

Kyle-Verhoog commented Jul 7, 2020 •

edited

Loading