Reform pythontests nixshell #391

jaoleal · 2025-02-28T21:37:41Z

What is the purpose of this pull request?

Which crates are being modified?

Description

Fixed: python tests nix-shell
* nix-shell: Added variation that doesnt run the tests.
* nix-shell: Explicit use of same version of rust than rust-toolchain.
* nix-shell: Added poe.
* prepare.sh now executes "poetry install --no-root" to set tests dependencies.
* floresta_cli_getrpcinfo.py now is path agnostic without its assertion on path tree.

Notes to the reviewers

The new runPythonTests variation was needed to provide a variation that doesnt execute the tests in its invocation, which is named pythonTests only

Checklist

I've signed all my commits
I ran just lint
I ran cargo test
I've checked the integration tests
I've followed the contribution guidelines
I'm linking the issue being fixed by this PR (if any)

* nix-shell: Added variation that doesnt run the tests. * nix-shell: Explicit use of same version of rust than rust-toolchain. * nix-shell: Added poe. * prepare.sh now executes "poetry install --no-root" to set tests dependencies. * floresta_cli_getrpcinfo.py now is path agnostic without its assertion on path tree.

flake.nix

justfile

jaoleal · 2025-03-04T10:39:30Z

justfile

+# Same as `test-int-nixsetup` but execute the tests and exits
+test-int-nixrun:
+    nix develop .#pythonTests -c bash -c "bash ./tests/run.sh"
+


@nyonson
instead of a raw call of "poetry run poe tests" i followed the same approach than CI.

run.sh just manipulates $PATH to expose the correct binary for python and, after running all tests, restores $PATH, of course.

its intrusive but works fine.

Are there isolation benefits to the double bash wrapping? Can we just make run.sh executable and call it directly or are you trying to avoid that given the PATH manipulation?

I hadn't thought of that... its just a weird habit that i have to avoid getting yelled by the terminal when i forget the shebang.
Since its already isolated by nix its safe to manipulate things there... Ill just change #!/bin/bash to #!/bin/sh

jaoleal · 2025-03-04T10:39:35Z

tests/test_framework/test_framework.py

-        # PR #331 introduced a preparatory environment at
-        # /tmp/floresta-integration-tests.$(git rev-parse HEAD).
-        tmpdir = FlorestaTestFramework.get_integration_test_dir()
-        targetdir = FlorestaTestFramework.get_target_release_dir()

-        # So, check for it first before define the florestad path.
-        if os.path.exists(tmpdir):
-            florestad = os.path.normpath(os.path.join(tmpdir, "florestad"))
-
-        # If not exists, define the one at ./target/release.
-        elif os.path.exists(targetdir):
-            florestad = os.path.normpath(os.path.join(targetdir, "florestad"))
-
-        # In case any test florestad is found, raise an exception
-        else:
-            raise RuntimeError(
-                f"Not found 'florestad' in '{tmpdir}' or '{targetdir}'. "
-                "Run 'tests/prepare.sh' or 'cargo build --release'."
-            )
-
-        print(f"Using {florestad}")
        setting = {
            "chain": chain,
            "config": [
-                florestad,
+                "florestad",


Here, i had to delete this piece made by @qlrd.

(Just publicizing a talk that i had with him via dm).

IMO this whole test battery shouldnt manipulate external binaries at all and just blindly call it from the environment. In this way, one that is running the tests can properly set the binaries. Thats what i tried to do with prepare.sh and run.sh.

nyonson · 2025-03-04T18:25:54Z

flake.nix

                go
-              ] ++ [ self.packages.${system}.default ];
+              ] ++ pythonDeps;


Some of these python deps are duplicated now right?

AFAIK poetry will not install already installed deps... But worth checking...

Ah ok, I see. I thought there were dups in the flake too, but I see how the python deps are spilt now.

nyonson · 2025-03-05T15:38:01Z

flake.nix

-              nixpkgs-fmt.enable = true;
-            };
-          };
+          #TODO: Add relevant checks here to be executed by CI with nix flake check


Are these related to the python stuff?

No, but this will leace space for some changes that was discussed in the last bi weekly reunion at our discord. Pre commit was only doing redundant job that Actions was already doing.

nyonson · 2025-03-05T16:02:28Z

@jaoleal I am trying to run this locally and still seeing a weird issue where it is prompting me for my root password. I think it is due to dependencies, but I see the "no-root" arg on poetry so confused where it is coming from. I'll keep poking around.

This is what I see when prompted:

$ just test-int-nixrun     
nix develop .#pythonTests -c bash -c "bash ./tests/run.sh"
The currently activated Python version 3.11.10 is not supported by the project (>=3.12).
Trying to find and use a compatible version. 
Using python3.12 (3.12.7)
Installing dependencies from lock file

Package operations: 21 installs, 0 updates, 0 removals

  - Installing astroid (3.3.6): Pending...
  - Installing certifi (2024.12.14): Pending...
  - Installing charset-normalizer (3.4.0): Pending...
  - Installing click (8.1.7): Pending...
  - Installing dill (0.3.9): Pending...
  - Installing idna (3.10): Pending...
  - Installing isort (5.13.2): Pending...
  - Installing mccabe (0.7.0): Pending...
  - Installing mypy-extensions (1.0.0): Pending...
  - Installing packaging (24.2): Pending...
  - Installing pastel (0.2.1): Pending...
  - Installing pathspec (0.12.1): Pending...
  - Installing platformdirs (4.3.6): Pending...
  - Installing pyyaml (6.0.2): Pending...
  - Installing tomlkit (0.13.2): Pending...
  - Installing urllib3 (2.2.3): Pending...
  - ```

nyonson · 2025-03-05T17:23:39Z

I am not sure if this is an option, but I got things working by just not using poetry:

diff --git a/flake.nix b/flake.nix
index 5c86db2..556f445 100644
--- a/flake.nix
+++ b/flake.nix
@@ -63,9 +63,7 @@
               _scriptSetup = ''bash ./tests/prepare.sh'';
               # Packages fetched from ./pyproject.toml
               pythonDeps = with python312Packages; [
-                black
                 requests
-                pylint
                 jsonrpc-base
               ];
             in
@@ -73,8 +71,6 @@
               buildInputs = with pkgs; [
                 florestaRust
                 python312
-                poetry
-                poethepoet
                 go
               ] ++ pythonDeps;
               shellHook = ''
diff --git a/tests/prepare.sh b/tests/prepare.sh
index 39e3677..0c931a0 100755
--- a/tests/prepare.sh
+++ b/tests/prepare.sh
@@ -35,17 +35,6 @@ then
        exit 1
 fi
 
-poetry -V  &>/dev/null
-
-if [ $? -ne 0 ]
-then
-       echo "You must have poetry installed to run those tests!"
-       exit 1
-fi
-
-poetry install --no-root
-
-
 ls $TEMP_DIR &>/dev/null
 if [ $? -ne 0 ]
 then
diff --git a/tests/run.sh b/tests/run.sh
index 28034b4..763044c 100755
--- a/tests/run.sh
+++ b/tests/run.sh
@@ -26,6 +26,6 @@ ORIGINAL_PATH=$PATH
 export PATH="$FLORESTA_BIN_DIR:$UTREEXO_BIN_DIR:$PATH"
 
 # Actually runs the tests
-poetry run poe tests
+python $FLORESTA_PROJ_DIR/tests/run_tests.py -t floresta_cli_getblock
 # Restores the original PATH
 export PATH=$ORIGINAL_PATH

qlrd · 2025-03-05T19:40:43Z

@nyonson, i would like to point a little thing if you permit:

but I see the "no-root" arg on poetry so confused where it is coming from.

The mentioned --no-root means ("Do not install the root package (your project)."). So is not about the OS's root. The poetry docs says that:

By default poetry will install your project’s package every time you run install:
If you want to skip this installation, use the --no-root option.
...
Similar to --no-root you can use --no-directory to skip directory path dependencies
...
This is mainly useful for caching in CI or when building Docker images.

Generally, without --no-root is intended for projects that will themselves be packages. Since Floresta's tests are not a package, there is no need to install the "root project" in the built virtual environment (located in poetry env info --path).

Maybe --no-directory can be most appropriate?

nyonson · 2025-03-05T20:15:09Z

@nyonson, i would like to point a little thing if you permit:

but I see the "no-root" arg on poetry so confused where it is coming from.

The mentioned --no-root means ("Do not install the root package (your project)."). So is not about the OS's root. The poetry docs says that:

By default poetry will install your project’s package every time you run install:
If you want to skip this installation, use the --no-root option.
...
Similar to --no-root you can use --no-directory to skip directory path dependencies
...
This is mainly useful for caching in CI or when building Docker images.

Generally, without --no-root is intended for projects that will themselves be packages. Since Floresta's tests are not a package, there is no need to install the "root project" in the built virtual environment (located in poetry env info --path).

Maybe --no-directory can be most appropriate?

Ah, thanks, I totally misinterpreted that flag. I am still not sure exactly why poetry makes things upset, but probably not the no-root flag.

jaoleal · 2025-03-05T22:21:18Z

I am not sure if this is an option, but I got things working by just not using poetry:
...

cc @nyonson

Im afraid not, i want nix to have similar behavior than our CI.

If CI fail, nix must fail too and vice versa.

nyonson · 2025-03-05T23:49:45Z

If CI fail, nix must fail too and vice versa.

That makes sense. I am poking around a few more strategies just out of curiosity and learning, but don't think they will make the cut unless CI is tweaked too.

This dev shell below technically works, but also shows off why python deps in nix is kind of painful. Some packages are in the python3 group and others (in this case, Poe) are not. Also it doesn't seem possible to control the exact version of a dependency since I think just one is hard-coded for reproduce-ability. Fun fact, in mkShell the packages keyword is an alias for nativeBuildInputs.

            pkgs.mkShell {
              packages = [
                (pkgs.python3.withPackages (python-pkgs: with python-pkgs; [
                  jsonrpc-base
                  requests
                  black
                  pylint
                ]))
                poethepoet
              ];
              shellHook = ''
                echo "Python integration test environment"
              '';
            };

Another option is this poetry2nix project. Although it looks like it is unmaintained at the moment (Looks like uv is the new hot python dependency manager?). But I think it would allow the pyproject.toml to be the source of truth and this would be nix reproduce-able.

If instead the requirements.txt is used as the source of truth, I think a dead simple (if not very nix-y due to lack of reproduce-ability) approach could be something like the following dev shell. But this would require some tweaks to CI I believe to match the source of truth?

        devShells.default = pkgs.mkShell {
          buildInputs = [
            pkgs.python3
            pkgs.python3Packages.pip
            pkgs.python3Packages.virtualenv
          ];
          
          shellHook = ''
            # Create a virtual environment if it doesn't exist
            if [ ! -d "venv" ]; then
              echo "Creating virtual environment..."
              virtualenv venv
            fi
            
            # Activate the virtual environment
            source venv/bin/activate
            
            # Install requirements if not already installed
            if [ -f requirements.txt ]; then
              echo "Installing packages from requirements.txt..."
              pip install -r requirements.txt
            fi
            
            echo "Integration test environment ready."
          '';
        };

qlrd · 2025-03-06T01:28:26Z

Another option is this poetry2nix project. Although it looks like it is unmaintained at the moment (Looks like uv is the new hot python dependency manager?). But I think it would allow the pyproject.toml to be the source of truth and this would be nix reproduce-able.

This uv appear to be a very interesting one and -- after a quick look at the docs -- can be compatible with the current poetry.

A try on it wouldn't hurt.

But this would require some tweaks to CI I believe to match the source of truth?

        shellHook = ''
            # Create a virtual environment if it doesn't exist
            if [ ! -d "venv" ]; then
              echo "Creating virtual environment..."
              virtualenv venv
            fi
            
            # Activate the virtual environment
            source venv/bin/activate
            
            # Install requirements if not already installed
            if [ -f requirements.txt ]; then
              echo "Installing packages from requirements.txt..."
              pip install -r requirements.txt
            fi
            
            echo "Integration test environment ready."
          '';

Basically, what poetry do is the same (create a named virtualenv for the "root project and install and install dependencies), but in long term, requirements.txt alone, can be annoying. I'm with this guy about some its issues:

requirements.txt is hardly even a standard, it just started as a convention at some point for listing dependencies - I don't know when or where it originated, though. Nowadays it's getting less common as dependency management has evolved, although older projects still use them.

setup.py (and setup.cfg) are what we used to use as build scripts for packages, but since it's inherently a security risk to run code as a build step we've since moved on to a standard that doesn't allow arbitrary code execution.

tox.ini is a configuration file for Tox, which is essentially a test runner mostly used in CI builds.

pyproject.toml combines all of the above, and more. It contains the project metadata, like dependencies and version number, and it can optionally be used for storing configuration for various tools used by the project, such as linters (Ruff), tests (Pytest, Tox), or build systems. It's the current standard, and is heavily inspired by Rust's cargo.toml files.

I believe that, with careful consideration, it would be possible to swap poetry for uv without sacrificing compatibility.

nyonson · 2025-03-06T03:51:18Z

Looks like there is a nice uv2nix project to do the reproducible mapping.

EDIT: Although, maybe worth just using poetry2nix right now cause this looks very simple: https://github.com/nix-community/poetry2nix?tab=readme-ov-file#mkpoetryenv

jaoleal · 2025-03-06T14:21:43Z

This dev shell below technically works, but also shows off why python deps in nix is kind of painful.

Allow me to correct your typo.

"shows off why python deps ~~in nix~~ is kind of painful. " 😆

I believe that, with careful consideration, it would be possible to swap poetry for uv without sacrificing compatibility.

uv looks a interesting alternative if its better, "blazingly fast" and possibly a drop-in solution for poetry. Also, even poetry2nix guys are recommending this change.

Ill be studying that option.

qlrd · 2025-03-06T14:27:49Z

I can try test with uv

nyonson · 2025-03-06T18:34:44Z

"shows off why python deps ~~in nix~~ is kind of painful. " 😆

Hehe yea, to your point, check out the "basic" usage of the uv2nix project...does enforce my bias to avoid python deps at almost all costs.

jaoleal force-pushed the reform-pythontests-nixshell branch from ee2153e to 04eec95 Compare February 28, 2025 21:40

jaoleal mentioned this pull request Feb 28, 2025

[Developer Env Bug] The pythonTests nix developer shell struggles with python dependencies #390

Open

nyonson reviewed Feb 28, 2025

View reviewed changes

flake.nix Show resolved Hide resolved

nyonson reviewed Feb 28, 2025

View reviewed changes

justfile Outdated Show resolved Hide resolved

jaoleal force-pushed the reform-pythontests-nixshell branch 2 times, most recently from 9fe57bb to ef46a8d Compare March 4, 2025 10:30

jaoleal commented Mar 4, 2025

View reviewed changes

Update justfile with python test nix-shell variation

4d84e64

jaoleal force-pushed the reform-pythontests-nixshell branch from ef46a8d to 4d84e64 Compare March 4, 2025 11:15

nyonson reviewed Mar 4, 2025

View reviewed changes

jaoleal requested a review from nyonson March 5, 2025 15:33

nyonson reviewed Mar 5, 2025

View reviewed changes

qlrd mentioned this pull request Mar 5, 2025

Added test for getblockheader JSONRPC call #398

Merged

21 tasks

This was referenced Mar 7, 2025

Feat: Functional tests. #252

Open

Re-structured functional tests. #402

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reform pythontests nixshell #391

Reform pythontests nixshell #391

jaoleal commented Feb 28, 2025

jaoleal Mar 4, 2025

jaoleal Mar 4, 2025 •

edited

Loading

nyonson Mar 4, 2025

jaoleal Mar 4, 2025

jaoleal Mar 4, 2025

nyonson Mar 4, 2025

jaoleal Mar 4, 2025

nyonson Mar 4, 2025

nyonson Mar 5, 2025

jaoleal Mar 5, 2025

nyonson commented Mar 5, 2025 •

edited

Loading

nyonson commented Mar 5, 2025

qlrd commented Mar 5, 2025 •

edited

Loading

nyonson commented Mar 5, 2025

jaoleal commented Mar 5, 2025 •

edited

Loading

nyonson commented Mar 5, 2025 •

edited

Loading

qlrd commented Mar 6, 2025 •

edited

Loading

nyonson commented Mar 6, 2025 •

edited

Loading

jaoleal commented Mar 6, 2025

qlrd commented Mar 6, 2025

nyonson commented Mar 6, 2025

Reform pythontests nixshell #391

Are you sure you want to change the base?

Reform pythontests nixshell #391

Conversation

jaoleal commented Feb 28, 2025

What is the purpose of this pull request?

Which crates are being modified?

Description

Notes to the reviewers

Checklist

Choose a reason for hiding this comment

jaoleal Mar 4, 2025 • 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

nyonson commented Mar 5, 2025 • edited Loading

nyonson commented Mar 5, 2025

qlrd commented Mar 5, 2025 • edited Loading

nyonson commented Mar 5, 2025

jaoleal commented Mar 5, 2025 • edited Loading

nyonson commented Mar 5, 2025 • edited Loading

qlrd commented Mar 6, 2025 • edited Loading

nyonson commented Mar 6, 2025 • edited Loading

jaoleal commented Mar 6, 2025

qlrd commented Mar 6, 2025

nyonson commented Mar 6, 2025

jaoleal Mar 4, 2025 •

edited

Loading

nyonson commented Mar 5, 2025 •

edited

Loading

qlrd commented Mar 5, 2025 •

edited

Loading

jaoleal commented Mar 5, 2025 •

edited

Loading

nyonson commented Mar 5, 2025 •

edited

Loading

qlrd commented Mar 6, 2025 •

edited

Loading

nyonson commented Mar 6, 2025 •

edited

Loading