Merge ~mertkirpici/juju-lint:lp/1989575 into juju-lint:master

[Mert Kirpici]

Close LP #1989575

After the renaming process in 30066d3b[1], makefile dev-environment target was overlooked, ending up with the broken target.

To fix this properly, rather than hardcoding tests/unit/requirements.txt into the makefile, we introduce requirements-dev.txt file and point the unittest requirements file to that.

Since this issue[0] is resolved, unpinned the flake8 version.

Also the .venv/ created by dev-environment was not ignored by git, fixed that.

[0] https://github.com/csachs/pyproject-flake8/issues/13
[1] https://git.launchpad.net/juju-lint/commit/?id=30066d3b9d49f4c673cf9766d13054b735c26a30

[Mert Kirpici (mertkirpici)]

make lint and unittests:
https://pastebin.ubuntu.com/p/gtcHgKD8Fx/

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Eric Chen (eric-chen)]

Please provide functional test log too.

review: Needs Information

[Mert Kirpici (mertkirpici)]

lint + unittests + functional:
https://pastebin.ubuntu.com/p/tF7CKBwzQz/

[Gabriel Cocenza (gabrielcocenza)]

I liked the idea of having a single requirements-dev.txt at the "principal" folder of the project.

With that said, is there a specific reason why the functional-tests requirements it's not in the same file?

If we have a single requirements-dev.txt it won't be necessary to have the unit and functional requirements.txt and the [testenv] on the .tox file could be responsible to install all dependencies of the project. With that, I guess we can remove deps from [testenv:func], [testenv:func-smoke], [testenv:func-dev] and [testenv:pre-commit].

review: Needs Information

[Mert Kirpici (mertkirpici)]

So after giving this some thought and experimenting, I came to a conclusion that having a central dev requirements ends up installing ALL of the dependencies for every tox environment, which is very redundant and takes a lot of time and space.

Therefore I moved away from that idea towards the opposite.
With the second change I pushed, every tox env installs only their own dependencies they need. (Most charms also follow this)

Also another change is that, the virtual env creation in makefile's `dev-environment` target was not being created by tox like all the rest but it was using the package `virtualenv`. I moved away from that also, in favor of using tox for the `dev-environment` creation as well.

Here is a showcase of all current make+tox targets :)
https://pastebin.ubuntu.com/p/ZkxG8mFsWV/

[Martin Kalcok (martin-kalcok)]

overall LGTM. just a small note. You can make multiple tox environments share the same "virtual environment folder" to cut down on venv duplicity. For example how "format-code" shares "lint" venv in juju-verify [0]. Same could be applied here.

[0] https://github.com/canonical/juju-verify/blob/237fea58fd579279121f476d244bacbd1992355c/tox.ini#L43

[Gabriel Cocenza (gabrielcocenza)]

Generally this patch LGTM, but the command `make pre-commit` it's not working.

review: Needs Information

[Mert Kirpici (mertkirpici)]

pushed update to recover the pre-commit target

[Gabriel Cocenza (gabrielcocenza)]

LGTM. Please provide the logs of tests before merging.

review: Approve

[Mert Kirpici (mertkirpici)]

updated one last time. changes:
- rebased onto current master.
- squashed commits.
- rewritten commit message

`make test`:
https://pastebin.ubuntu.com/p/WdZ8q9j5nC/

all green. ready to land.

[Eric Chen (eric-chen)]

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision c05a3913ecaabbc5ba41cf14895ffb7ced946567

diff --git a/.gitignore b/.gitignore
index 764c99f..b095e7e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -31,3 +31,4 @@ output/
 
 # venv
 /venv
+.venv
diff --git a/Makefile b/Makefile
index 4857f3f..7c33d38 100644
--- a/Makefile
+++ b/Makefile
@@ -45,9 +45,7 @@ clean:
 
 dev-environment:
 	@echo "Creating virtualenv and installing pre-commit"
-	@virtualenv -p python3 .venv
-	@.venv/bin/pip install -r tests/requirements.txt
-	@.venv/bin/pre-commit install
+	@tox -r -e dev-environment
 
 functional: build
 	@echo "Executing functional tests using built snap"
diff --git a/tests/functional/requirements.txt b/tests/functional/requirements.txt
index 2220b6c..f7fed82 100644
--- a/tests/functional/requirements.txt
+++ b/tests/functional/requirements.txt
@@ -1 +1,2 @@
-pytest-operator
\ No newline at end of file
+pytest
+pytest-operator
diff --git a/tests/unit/requirements.txt b/tests/unit/requirements.txt
index ce434c0..0710dca 100644
--- a/tests/unit/requirements.txt
+++ b/tests/unit/requirements.txt
@@ -1,16 +1,5 @@
-# Module requirements
-black
-colorama
-flake8<5.0  # some flake8 plugins seem to be currently broken with version 5.0+
-flake8-colors
-flake8-docstrings
-flake8-html
-isort
-mock
-pep8-naming
-pyflakes
 pytest
 pytest-cov
 pytest-html
 pytest-mock
-pre-commit
+mock
diff --git a/tox.ini b/tox.ini
index a89fd0b..ba747d8 100644
--- a/tox.ini
+++ b/tox.ini
@@ -17,11 +17,11 @@ skip_missing_interpreters = True
 
 [testenv]
 basepython = python3.8
-deps =
-    -r{toxinidir}/tests/unit/requirements.txt
-    -r{toxinidir}/requirements.txt
 
 [testenv:unit]
+deps =
+    -r{toxinidir}/requirements.txt
+    -r{toxinidir}/tests/unit/requirements.txt
 commands =
     pytest -v \
     --cov=jujulint \
@@ -38,11 +38,9 @@ commands =
 setenv = PYTHONPATH={toxinidir}
 
 [testenv:func]
+deps = -r{toxinidir}/tests/functional/requirements.txt
 passenv =
     JUJULINT_TEST_*
-deps =
-    {[testenv]deps}
-    -r{toxinidir}/tests/functional/requirements.txt
 commands =
     pytest -v \
     --log-cli-level=WARNING \
@@ -51,8 +49,8 @@ commands =
 
 [testenv:func-smoke]
 deps =
-    {[testenv]deps}
-    -r{toxinidir}/tests/functional/requirements.txt
+    {[testenv:func]deps}
+    -r{toxinidir}/requirements.txt
 commands =
     pytest -v \
     --log-cli-level=WARNING \
@@ -61,26 +59,36 @@ commands =
 
 [testenv:func-dev]
 deps =
-    {[testenv]deps}
-    -r{toxinidir}/tests/functional/requirements.txt
+    {[testenv:func]deps}
+    -r{toxinidir}/requirements.txt
 commands =
     pytest -v \
     --log-cli-level=WARNING \
     --ignore={toxinidir}/tests/unit
 
 [testenv:lint]
+deps =
+    black
+    colorama
+    flake8
+    flake8-colors
+    flake8-docstrings
+    flake8-html
+    isort
+    pep8-naming
+    pyflakes
+
 commands =
     flake8 --color always
     black --check --diff --color .
     isort --check-only --diff --color .
 
-
 [testenv:reformat]
+deps = {[testenv:lint]deps}
 commands =
     black .
     isort .
 
-
 [pytest]
 filterwarnings =
     ignore::DeprecationWarning
@@ -99,7 +107,18 @@ skip_glob =
     mod,
     build,
 
+[testenv:dev-environment]
+deps =
+    pre-commit
+    {[testenv:lint]deps}
+    {[testenv:unit]deps}
+    {[testenv:func]deps}
+envdir = {toxinidir}/.venv
+commands =
+    pre-commit install
+
 [testenv:pre-commit]
-deps = pre-commit
+deps = {[testenv:dev-environment]deps}
+envdir = {[testenv:dev-environment]envdir}
 commands =
     pre-commit run -a