Separate `tox.ini` into multiple files #2508

ocelotl · 2024-05-08T14:50:26Z

This is still a draft PR because I have not yet refactored all packages so that all of them have their own tox.ini file.

The advantages of this proposal are:

The main tox.ini file will be much smaller. Right now, this file is extremely big.
The tox commands will be much simpler. Instead of running tox -e py38-test-opentelemetry-name-of-the-package we will be able to just cd into the package directory and then run tox -e test-py38.
This PR will add coverage testing for all packages.
Right now we run pytest with the benchmark options active for all tests, even when only one package here has benchmark tests. This introduces a warning in all of our logs. We should only run pytest with the benchmark options when there are benchmark tests, this PR fixes this issue.
Right now, we should not run coverage nor benchmark tests in CI (please read the comments below for an explanation). This PR allows us to not run these tests in CI.
Having a separate tox.ini file per package allows users to simply run tox in the package they changed to run all necessary tests.

.github/workflows/instrumentations_0.yml

ocelotl · 2024-05-08T17:55:28Z

.github/workflows/lint.yml

@@ -68,8 +68,6 @@ jobs:
          - "processor-baggage"
          - "propagator-aws-xray"
          - "propagator-ot-trace"
-          - "resource-detector-container"


I am using these 2 packages as an example.

ocelotl · 2024-05-08T17:56:43Z

.github/workflows/lint_1.yml

+            ~/.cache/pip
+          key: v7-build-tox-cache-${{ env.RUN_MATRIX_COMBINATION }}-${{ hashFiles('tox.ini', 'gen-requirements.txt', 'dev-requirements.txt') }}
+      - name: run tox
+        run: tox -c ${{ matrix.package }}/tox.ini -e lint


Notice how we are running tox using a specific tox.ini file, we use this pattern to avoid having to cd into every package directory and back.

ocelotl · 2024-05-08T17:57:59Z

.gitignore

 coverage.xml
 .coverage
 .nox
 .tox
 .cache
 htmlcov
+benchmark.json


This file is generated when benchmark tests are run in the local computer of a developer. Added in this file to avoid adding it to git unintentionally.

ocelotl · 2024-05-08T17:59:04Z

.isort.cfg

@@ -14,6 +14,6 @@ profile=black
 ; docs: https://github.com/timothycrosley/isort#multi-line-output-modes
 multi_line_output=3
 skip=target
-skip_glob=**/gen/*,.venv*/*,venv*/*,.tox/*
+skip_glob=**/gen/*,.venv*/*,venv*/*,.tox/*,**/.tox/*


Since new tox.ini files are being added, new .tox directories will be generated into every package when tox is executed locally. This tells isort to not check these new .tox directories.

ocelotl · 2024-05-08T18:05:11Z

resource/opentelemetry-resource-detector-container/tox.ini

+  lint: pylint --rcfile={toxinidir}/../../.pylintrc {toxinidir}/src/opentelemetry
+  lint: pylint --rcfile={toxinidir}/../../.pylintrc {toxinidir}/tests
+
+  coverage: coverage erase


Using the coverage commands directly here, so that it is not necessary to check an additional file (scripts/coverage/sh) to understand what coverage tests are doing.

ocelotl · 2024-05-08T18:06:01Z

resource/opentelemetry-resource-detector-container/tox.ini

+  coverage: coverage report --show-missing
+  coverage: coverage xml
+
+  spellcheck: codespell --config {toxinidir}/../../.codespellrc {toxinidir}


spellcheck can now be run only for a certain package instead for the whole repo, making it faster for most changes.

ocelotl · 2024-05-08T18:09:05Z

sdk-extension/opentelemetry-sdk-extension-aws/tox.ini

+
+  spellcheck: codespell --config {toxinidir}/../../.codespellrc {toxinidir}
+
+  benchmark: pytest {toxinidir}/benchmarks --benchmark-json=benchmark.json


Notice how the benchmark tests are now in their own directory, not inside tests. This makes it possible to run them separately and not run them in CI.

ocelotl · 2024-05-08T18:12:46Z

tox.ini

@@ -1169,8 +1126,6 @@ commands =
  lint-exporter-prometheus-remote-write: pylint {toxinidir}/exporter/opentelemetry-exporter-prometheus-remote-write/src/opentelemetry
  lint-exporter-prometheus-remote-write: pylint {toxinidir}/exporter/opentelemetry-exporter-prometheus-remote-write/tests

-  coverage: {toxinidir}/scripts/coverage.sh


Right now, we should not run coverage nor benchmark tests in CI. As they are configured right now, the coverage tests will not fail if a decrease in coverage is detected, the benchmark test will not fail if a decrease in performance is detected. For this reason, they cannot fail CI in a useful manner and it makes no sense to spend resources running them.

For these tests to be usefully executed in CI we need to configure them so that they can fail CI if a decrease in performance (for the benchmark tests) or a decrease in coverage (for the coverage tests) is detected.

👍 makes sense. We should a file a bug regardless of this PR to set up coverage in CI

ocelotl · 2024-05-08T18:13:34Z

tox.ini

@@ -1120,20 +1091,6 @@ commands =

  test-util-http: pytest {toxinidir}/util/opentelemetry-util-http/tests {posargs}

-  test-sdk-extension-aws: pytest {toxinidir}/sdk-extension/opentelemetry-sdk-extension-aws/tests {posargs}


The size of this tox.ini file will be greatly reduced when the rest of the packages are refactored.

lzchen · 2024-05-24T19:29:31Z

I'm not really super enthusiastic about having so many tox files. Is the largeness of the singular tox file so much that it warrants separating these out?

From the issue:

Right now, tox.ini is extremely long and it is easy to overlook important testing details.

What "important testing details" are you referring to specifically? Maybe if we call them out, there are smaller, less intrusive changes we can make.

ocelotl · 2024-05-28T18:39:57Z

I'm not really super enthusiastic about having so many tox files. Is the largeness of the singular tox file so much that it warrants separating these out?

From the issue:

Right now, tox.ini is extremely long and it is easy to overlook important testing details.

What "important testing details" are you referring to specifically? Maybe if we call them out, there are smaller, less intrusive changes we can make.

When I was introducing the changes that split lint into multiple tests, I noticed that we were missing several missing testing for a few of our components. It was hard to notice among the hundreds of lines in our tox.ini.

Nevertheless, splitting the tox.ini file is just another enhancement that this PR will introduce, please take a look at the advantages listed in this PR description.

aabmass · 2024-06-06T17:10:29Z

My main concern with this approach is that the common parts of the config that all tox.ini share may drift over time and become inconsistent or difficult to maintain. For example, adding/removing a supported Python version will require updating every tox file. More subtle things like changing the flags to a command or adding a new tool would be tricky.

IIRC tox doesn't really have any mechanism for re-use like include/extend?

ocelotl · 2024-06-11T20:07:40Z

My main concern with this approach is that the common parts of the config that all tox.ini share may drift over time and become inconsistent or difficult to maintain. For example, adding/removing a supported Python version will require updating every tox file.

That's right, but we have the very same problem now. We can forget to add a new python version in some package in the tox.ini file as it is right now too. Having one single tox.ini file is no guarantee of consistency either.

More subtle things like changing the flags to a command or adding a new tool would be tricky.

Yes, but again, we have the same problem right now, we can make a mistake and forget a command argument for a particular package even if we only have one tox.ini file.

tox.ini

ocelotl · 2024-07-05T19:06:22Z

#2666, we have typos in our tox.ini file.

This reverts commit ef1680a.

This reverts commit b45806c.

This reverts commit 8c4de67.

ocelotl · 2024-07-10T19:26:52Z

Another problem we have with a big tox.ini file is that we can't have consistent tox environment names.

We need py3{8,9,10,11,12}-test-opentelemetry-instrumentation and py3{8,9,10,11,12}-test-instrumentation-aiohttp-client instead of py3{8,9,10,11,12}-test-opentelemetry-instrumentation-aiohttp-client because we have commands like this:

test-opentelemetry-instrumentation: pytest {toxinidir}/opentelemetry-instrumentation/tests {posargs}

That command above would incorrectly match py3{8,9,10,11,12}-test-opentelemetry-instrumentation-aiohttp-client too.

We can save ourselves the trouble of handling these collisions by having one tox.ini file per package.

ocelotl added the Skip Changelog PRs that do not require a CHANGELOG.md entry label May 8, 2024

ocelotl self-assigned this May 8, 2024

github-actions bot assigned NathanielRN May 8, 2024

ocelotl force-pushed the issue_2478 branch from bbaad16 to 7c5ad67 Compare May 8, 2024 17:54

ocelotl commented May 8, 2024

View reviewed changes

ocelotl changed the title ~~WIP~~ Separate tox.ini into multiple files May 8, 2024

ocelotl requested review from aabmass and lzchen May 24, 2024 13:41

ocelotl force-pushed the issue_2478 branch from e56584b to 1377246 Compare June 11, 2024 19:58

ocelotl force-pushed the issue_2478 branch from 1377246 to d2c7234 Compare June 11, 2024 20:10

xrmx reviewed Jun 13, 2024

View reviewed changes

tox.ini Outdated Show resolved Hide resolved

ocelotl force-pushed the issue_2478 branch from d2c7234 to 6f8a996 Compare June 13, 2024 17:19

ocelotl force-pushed the issue_2478 branch 10 times, most recently from 8e7d465 to 9548cc9 Compare July 5, 2024 17:40

ocelotl added 3 commits July 8, 2024 11:48

WIP

98bb139

Automate workflows

9d8f27d

Try with GITHUB_OUTPUT

8e1a855

ocelotl added 19 commits July 8, 2024 11:48

Try again with old commands

81319f8

WUP

d708086

try getting os from matrix

9bdff8f

try with tiwndos

51f6f91

wet

fb6ee41

fix

a0aaa50

WUP

3431f6e

sdf

408a52c

we

00a7396

sdf

d0e43a3

sdfsd

dd45b8f

Try different job names

7fe0eac

Revert "Try different job names"

309d3e1

This reverts commit ef1680a.

try with run-name

77419ab

Revert "try with run-name"

6470b24

This reverts commit b45806c.

try with run name

a6e818c

Revert "try with run name"

7135c7b

This reverts commit 8c4de67.

Use name for job

0a8ce77

Rename variables

7bf40b5

ocelotl force-pushed the issue_2478 branch from 622666f to c08974b Compare July 8, 2024 17:48

Change name

f2b56b7

ocelotl force-pushed the issue_2478 branch from c08974b to f2b56b7 Compare July 8, 2024 17:54

ocelotl added 4 commits July 8, 2024 15:42

Try with GITHUB_OUTPUT

d5c5d69

Use outputs

db4677c

Try run name

7c721cf

Rename to tox_ini_directory

02e36b1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate `tox.ini` into multiple files #2508

Separate `tox.ini` into multiple files #2508

ocelotl commented May 8, 2024 •

edited

Loading

ocelotl May 8, 2024

ocelotl May 8, 2024

ocelotl May 8, 2024

ocelotl May 8, 2024

ocelotl May 8, 2024

ocelotl May 8, 2024

ocelotl May 8, 2024

ocelotl May 8, 2024

aabmass Jun 13, 2024

ocelotl May 8, 2024

lzchen commented May 24, 2024

ocelotl commented May 28, 2024

aabmass commented Jun 6, 2024

ocelotl commented Jun 11, 2024

ocelotl commented Jul 5, 2024

ocelotl commented Jul 10, 2024


		spellcheck: codespell --config {toxinidir}/../../.codespellrc {toxinidir}

		benchmark: pytest {toxinidir}/benchmarks --benchmark-json=benchmark.json

		@@ -1120,20 +1091,6 @@ commands =

		test-util-http: pytest {toxinidir}/util/opentelemetry-util-http/tests {posargs}

		test-sdk-extension-aws: pytest {toxinidir}/sdk-extension/opentelemetry-sdk-extension-aws/tests {posargs}

Separate tox.ini into multiple files #2508

Are you sure you want to change the base?

Separate tox.ini into multiple files #2508

Conversation

ocelotl commented May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzchen commented May 24, 2024

ocelotl commented May 28, 2024

aabmass commented Jun 6, 2024

ocelotl commented Jun 11, 2024

ocelotl commented Jul 5, 2024

ocelotl commented Jul 10, 2024

Separate `tox.ini` into multiple files #2508

Separate `tox.ini` into multiple files #2508

ocelotl commented May 8, 2024 •

edited

Loading