refactor tests and dev env #422

kingarrrt · 2023-11-28T13:12:54Z

This attempts to extend and clean up the test suite and dev environment. Not complete but a good start (I think) and I hope not controversial. I was surprised that you are not dogfooding with an .envrc, and have added that in.

The tests are now self-contained, so unaffected by external state, and equally they won't mess with your $HOME. The bear in the corner remains - gc tests still use the real store, which is painful.

One thing this flags up is your minimum nix requirement. Nix 2.4 was removed from Nixpkgs in July/22. The lowest version you can support in Nixpkgs is 2.10, but this has it's own problems, see the last commit. Maybe time to bump the requirement?

* fix deprecation warnings * define dependencies once * expose all supported nix versions in nixpkgs for test * use ruff for both check and format, dropping black

* factor out duplicated code * make tests an importable package * idiomatic pytest usage * do not touch files outside of the tmp test tree and do not depend on external state (except for /nix/store ☹) * run coverage to root out dead code

drop redundant configuration

* test envs have a local gc root under ./._test_gcroot * update github workflow to run tests on a matrix of supported nix versions and ubuntu/macos * use jrmurr/direnv-nix-action in github workflow

bbenne10 · 2023-11-28T13:29:21Z

Hah. I was just thinking that I would like to get the tests working more transparently (and figure out why they aren't running on #420?). I'll look this over soon.

kingarrrt · 2023-11-28T13:44:39Z

Er, I've got two more branches, https://github.com/kingarrrt/nix-direnv/tree/cleanup and https://github.com/kingarrrt/nix-direnv/tree/cached. The first is a general cleanup, the second re-implements the cache keyed on the hash of watched files. Unfortunately each builds on the last, including this one, and I can't create pull requests without a bunch of duplicate commits. I thought Github would deal with the situation but no. I'm not quite sure what to do. This was an attempt an making things easier to review...

Re the failures: I hadn't realized Garnix ran flake check. I've got to head out but will come back later and move these tests to be exposed under something that won't get picked by it, since the tests are still impure so cannot work in this context.

Re #420: I ended up doing something similar on the cached branch, sorry to step on your toes.

bbenne10 · 2023-11-28T17:16:05Z

Frankly, I think these branches are too big and leading to PRs that are much too large for easy review. I'd be telling developers at work the same thing
Looking at your cached branch, I see a bunch of potential PRs that sort of build on one another (these are probably out of order):

Add .envrc to dogfood ./direnvrc in development
Test & Entrypoint refactor
Python modernization
Use_flake tests
Hash watched files (this actually addresses hash all the files #299, I think?)
Only create manual reload script if necessary (See note below on this one)
Factor out _nix command and modernize nix invocations (This is where you'd drop nix_direnv_realpath)
Correct typos in README
Improve manual reloading (this one's a doozy and definitely deserves a standalone PR)
And so on

I am happy to review MORE pull requests, but the code here is complex enough that I don't want to review one MASSIVE pull request doing all of this as the potential for missteps in reviewing one big PR is much higher than multiple small(er) ones. I might suggest, actually, closing this one and starting with smaller PRs. It isn't completely necessary, but it does simplify PR creation, management, and review.

Re: conditional manual reload script creation: We decided to ALWAYS create the manual reload script. It's simpler to do and there's no real drawback to creating the script itself. You're welcome to open a PR to re-open discussion on this, but I don't see a reason for my opinion to change yet.

Mic92 · 2023-11-29T06:41:07Z

tests/test_use_flake.py

@@ -0,0 +1,37 @@
+from __future__ import annotations


Do we still need this? We don't need to support python 3.7 anymore for our internal test suite.

Mic92 · 2023-11-29T06:42:06Z

pyproject.toml


+[tool.coverage.report]


Only our test code is using python, so I don't think we need code coverage for this.

Mic92 · 2023-11-29T06:42:48Z

default.nix

+        nix
+      ] ++ (with python3.pkgs; [
+        pytestCheckHook
+        pytest-cov


So we might not need this?

Mic92 · 2023-11-29T06:43:11Z

default.nix

  installPhase = ''
    install -m400 -D direnvrc $out/share/nix-direnv/direnvrc
  '';

+  passthru.tests = {
+    lint = runCommand "lint-${finalAttrs.name}" {


I got rid of lint now in favor of treefmt.

Mic92 · 2023-11-29T06:44:33Z

default.nix

+        pytestCheckHook
+        pytest-cov
+        pytest-randomly
+        pytest-sugar


Checked the number of dependencies and it seems reasonable.

Mic92 · 2023-11-29T06:47:03Z

There are certainly some good things in this PR but I also would like to take this in a smaller scoped pull requests.

Mic92 · 2023-11-29T07:39:18Z

I wanted to make myself familiar with these new lints, so I applied them myself in #425

That hopefully also reduce the scope of this PR.

kingarrrt · 2023-11-29T10:54:50Z

Understood. I'll split this down and resubmit.

kingarrrt added 7 commits November 28, 2023 04:26

dogfooding: use direnv

ee4ba4a

refactor flake, shell, lint, and test entry

c3c2345

* fix deprecation warnings * define dependencies once * expose all supported nix versions in nixpkgs for test * use ruff for both check and format, dropping black

refactor tests

0d145c0

* factor out duplicated code * make tests an importable package * idiomatic pytest usage * do not touch files outside of the tmp test tree and do not depend on external state (except for /nix/store ☹) * run coverage to root out dead code

move all py tool config to pyproject.toml

21b72cc

drop redundant configuration

create a Makefile to ease usage for lint and tests

b3f4a75

* test envs have a local gc root under ./._test_gcroot * update github workflow to run tests on a matrix of supported nix versions and ubuntu/macos * use jrmurr/direnv-nix-action in github workflow

add basic flake tests + fix unset variable caught by them

9e0447a

work around nix 2.10 bug

2b3079c

Mic92 reviewed Nov 29, 2023

View reviewed changes

pyproject.toml

[tool.coverage.report]

Copy link

Member

Mic92 Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only our test code is using python, so I don't think we need code coverage for this.

Mic92 reviewed Nov 29, 2023

View reviewed changes

default.nix

nix

] ++ (with python3.pkgs; [

pytestCheckHook

pytest-cov

Copy link

Member

Mic92 Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we might not need this?

Mic92 reviewed Nov 29, 2023

View reviewed changes

default.nix

pytestCheckHook

pytest-cov

pytest-randomly

pytest-sugar

Copy link

Member

Mic92 Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the number of dependencies and it seems reasonable.

kingarrrt closed this Nov 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor tests and dev env #422

refactor tests and dev env #422

kingarrrt commented Nov 28, 2023 •

edited

Loading

bbenne10 commented Nov 28, 2023

kingarrrt commented Nov 28, 2023 •

edited

Loading

bbenne10 commented Nov 28, 2023

Mic92 Nov 29, 2023

Mic92 Nov 29, 2023

Mic92 Nov 29, 2023

Mic92 Nov 29, 2023

Mic92 Nov 29, 2023

Mic92 commented Nov 29, 2023

Mic92 commented Nov 29, 2023 •

edited

Loading

kingarrrt commented Nov 29, 2023

refactor tests and dev env #422

refactor tests and dev env #422

Conversation

kingarrrt commented Nov 28, 2023 • edited Loading

bbenne10 commented Nov 28, 2023

kingarrrt commented Nov 28, 2023 • edited Loading

bbenne10 commented Nov 28, 2023

Mic92 Nov 29, 2023

Choose a reason for hiding this comment

Mic92 Nov 29, 2023

Choose a reason for hiding this comment

Mic92 Nov 29, 2023

Choose a reason for hiding this comment

Mic92 Nov 29, 2023

Choose a reason for hiding this comment

Mic92 Nov 29, 2023

Choose a reason for hiding this comment

Mic92 commented Nov 29, 2023

Mic92 commented Nov 29, 2023 • edited Loading

kingarrrt commented Nov 29, 2023

kingarrrt commented Nov 28, 2023 •

edited

Loading

kingarrrt commented Nov 28, 2023 •

edited

Loading

Mic92 commented Nov 29, 2023 •

edited

Loading