lf- / nix-doc

An interactive Nix documentation tool providing a CLI for function search, a Nix plugin for docs in the REPL, and a ctags implementation for Nix script
201 stars 6 forks source link

Unit test `tags::tests::smoke` is broken in main #24

Closed mfrischknecht closed 11 months ago

mfrischknecht commented 11 months ago

Currently, running the unit tests for nix-doc fails on tags::tests::smoke, which in turn also breaks the build for nix-doc in Nixpkgs. It seems like the latest changes (dealing with the order of tags, from what I understand at a glance) don't match the actual output of the tool.

I've verified the issue by compiling and running the tests manually, too, to make sure that this is not an artifact of some inner workings in Nixpkgs (although I'm not sure if it might still be due to a change in a recent nix version; from what I can see this is a dependency (nix-expr), and I simply ran the tests on my NixOS machine without checking multiple versions).

I'm not familiar enough with the tool's output and what the unit test actually is supposed to test, which makes me hesitant to propose a change to the expected string.

Example for a failed Hydra build: https://hydra.nixos.org/build/242152066 Hydra log: https://hydra.nixos.org/build/242152066/nixlog/1

Relevant log excerpt:

error: expect test failed
   --> nix-doc/src/tags.rs:464:13

You can update all `expect!` tests by running:

    env UPDATE_EXPECT=1 cargo test

To update a single test, place the cursor on `expect` token and use `run` feature of rust-analyzer.

Expect:
----
!_TAG_FILE_FORMAT       2       /extended format/
!_TAG_FILE_SORTED       1       /0=unsorted, 1=sorted, 2=foldcase/
!_TAG_FILE_ENCODING     utf-8   //
!_TAG_PROGRAM_NAME      nix-doc tags    //
!_TAG_PROGRAM_URL       https://github.com/lf-/nix-doc  //
c       testdata/test.nix       /^   a.b.c = a: 1;$/;"  f
c       testdata/test.nix       /^   c = {$/;"  m
ff      testdata/test.nix       /^   inherit ff;$/;"    m
fixedWidthString        testdata/regression-11.nix      /^  fixedWidthString = width: filler: str:$/;"  f
grub    testdata/test.nix       /^   inherit (n) grub hello;$/;"        m
hello   testdata/test.nix       /^   inherit (n) grub hello;$/;"        m
the-fn  testdata/test.nix       /^   the-fn = a: b: {z = a; y = b;};$/;"        f
the-fn  testdata/test.nix       /^    the-fn = a: a;$/;"        f
the-fn  testdata/test.nix       /^    the-fn = a: a;$/;"        f
the-fn  testdata/test2.nix      /^  inherit the-fn;$/;" m
the-fn  testdata/test.nix       /^    inherit the-fn;$/;"       m
the-snd-fn      testdata/test.nix       /^   the-snd-fn = {b, \/* doc *\/ c}: {};$/;"   f
withFeature     testdata/regression-11.nix      /^  withFeature = with_: feat: "--\${if with_ then "with" else "without"}-\${feat}";$/;"        f
withFeatureAs   testdata/regression-11.nix      /^  withFeatureAs = with_: feat: value: withFeature with_ feat + optionalString with_ "=\${value}";$/;" f
x       testdata/test.nix       /^   x = {$/;"  m
y       testdata/test.nix       /^   y = {$/;"  m
y       testdata/test.nix       /^   the-fn = a: b: {z = a; y = b;};$/;"        m
z       testdata/test.nix       /^   the-fn = a: b: {z = a; y = b;};$/;"        m
----

Actual:
----
!_TAG_FILE_FORMAT       2       /extended format/
!_TAG_FILE_SORTED       1       /0=unsorted, 1=sorted, 2=foldcase/
!_TAG_FILE_ENCODING     utf-8   //
!_TAG_PROGRAM_NAME      nix-doc tags    //
!_TAG_PROGRAM_URL       https://github.com/lf-/nix-doc  //
c       testdata/test.nix       /^   a.b.c = a: 1;$/;"  f
c       testdata/test.nix       /^   c = {$/;"  m
ff      testdata/test.nix       /^   inherit ff;$/;"    m
fixedWidthString        testdata/regression-11.nix      /^  fixedWidthString = width: filler: str:$/;"  f
grub    testdata/test.nix       /^   inherit (n) grub hello;$/;"        m
hello   testdata/test.nix       /^   inherit (n) grub hello;$/;"        m
the-fn  testdata/test.nix       /^   the-fn = a: b: {z = a; y = b;};$/;"        f
the-fn  testdata/test.nix       /^    the-fn = a: a;$/;"        f
the-fn  testdata/test.nix       /^    the-fn = a: a;$/;"        f
the-fn  testdata/test.nix       /^    inherit the-fn;$/;"       m
the-fn  testdata/test2.nix      /^  inherit the-fn;$/;" m
the-snd-fn      testdata/test.nix       /^   the-snd-fn = {b, \/* doc *\/ c}: {};$/;"   f
withFeature     testdata/regression-11.nix      /^  withFeature = with_: feat: "--\${if with_ then "with" else "without"}-\${feat}";$/;"        f
withFeatureAs   testdata/regression-11.nix      /^  withFeatureAs = with_: feat: value: withFeature with_ feat + optionalString with_ "=\${value}";$/;" f
x       testdata/test.nix       /^   x = {$/;"  m
y       testdata/test.nix       /^   y = {$/;"  m
y       testdata/test.nix       /^   the-fn = a: b: {z = a; y = b;};$/;"        m
z       testdata/test.nix       /^   the-fn = a: b: {z = a; y = b;};$/;"        m
----

Diff:
----
!_TAG_FILE_FORMAT       2       /extended format/
!_TAG_FILE_SORTED       1       /0=unsorted, 1=sorted, 2=foldcase/
!_TAG_FILE_ENCODING     utf-8   //
!_TAG_PROGRAM_NAME      nix-doc tags    //
!_TAG_PROGRAM_URL       https://github.com/lf-/nix-doc  //
c       testdata/test.nix       /^   a.b.c = a: 1;$/;"  f
c       testdata/test.nix       /^   c = {$/;"  m
ff      testdata/test.nix       /^   inherit ff;$/;"    m
fixedWidthString        testdata/regression-11.nix      /^  fixedWidthString = width: filler: str:$/;"  f
grub    testdata/test.nix       /^   inherit (n) grub hello;$/;"        m
hello   testdata/test.nix       /^   inherit (n) grub hello;$/;"        m
the-fn  testdata/test.nix       /^   the-fn = a: b: {z = a; y = b;};$/;"        f
the-fn  testdata/test.nix       /^    the-fn = a: a;$/;"        f
the-fn  testdata/test.nix       /^    the-fn = a: a;$/;"        f
the-fn  testdata/test2.nix      /^    inherit the-fn;$/;"       m
the-fn  testdata/test2.nix      /^    inherit the-fn;$/;"       m
the-snd-fn      testdata/test.nix       /^   the-snd-fn = {b, \/* doc *\/ c}: {};$/;"   f
withFeature     testdata/regression-11.nix      /^  withFeature = with_: feat: "--\${if with_ then "with" else "without"}-\${feat}";$/;"        f
withFeatureAs   testdata/regression-11.nix      /^  withFeatureAs = with_: feat: value: withFeature with_ feat + optionalString with_ "=\${value}";$/;" f
x       testdata/test.nix       /^   x = {$/;"  m
y       testdata/test.nix       /^   y = {$/;"  m
y       testdata/test.nix       /^   the-fn = a: b: {z = a; y = b;};$/;"        m
z       testdata/test.nix       /^   the-fn = a: b: {z = a; y = b;};$/;"        m
----

failures:
    tags::tests::smoke

test result: FAILED. 6 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s
mfrischknecht commented 11 months ago

Here's a colored version of the diff output:

image

lf- commented 11 months ago

Man, I should implement CI to not make a fool of myself! I'll fix it shortly, thanks for the heads up.

lf- commented 11 months ago

oooooh, ok, that's a very interesting diff. Definitely a bug in nix-doc (I am evidently not sorting on enough fields so something being unstable-sorted breaks stuff), but I cannot possibly fathom how it became a hydra failure, when none of the code involved is anything outside "rust std and pinned rust libs".

The reference to libnixexpr is a red herring: we do depend on libnixexpr, but it's only for the nix plugin, not anything in ctags.

Even worse, I think your hydra failure might be caused by external factors because I just checked out the latest nixpkgs master, and it builds fine :sob: So this is probably some kind of fucked up concurrency bug that always landed one way in the past.

Thank you again for reporting this, I think I have enough to go off of to fix it.

lf- commented 11 months ago

I have a fix plan, but I will have to work on it some more tomorrow, since it looks like my current implementation of it severely regresses on performance compared to the previous implementation, and almost certainly not for a good reason.

mfrischknecht commented 11 months ago

Even worse, I think your hydra failure might be caused by external factors because I just checked out the latest nixpkgs master, and it builds fine 😭 So this is probably some kind of fucked up concurrency bug that always landed one way in the past.

Sorry for the late answer; it only just occurred to me that it might be useful if I check if the tests do indeed fail deterministically on my machine. And turns out: I was very "lucky" that they failed on my first try. Even on my machine they are green way more often than not.

To test the repeatability of the failure some more, I ran the check in a loop a couple of times (bash: time (while true; do cargo test || break; done)), and it can really take a while until I see a failure. I'm also pretty sure that keeping the CPU busy (e.g. nix-shell -p p7zip --run '7z b') makes the error much more likely to occur (in my case, the loop aborted after single-digit second delays instead of ~40s if the CPU is idle). In retrospect, that also might explain the repeated occurrences on Hydra.

I hope that's helpful :)

If it's still not as likely on your hardware as it is on mine, I can of course re-run the tests for a specific branch if you need me to.

lf- commented 11 months ago

No worries, I know why it is: basically we weren't sorting on file name or location within a file. I added such sorting, which regresses time by about 25% (much better than the 50% i had earlier but still kinda unfortunate). Going to commit a release in a bit.

lf- commented 11 months ago

Fixed by https://github.com/lf-/nix-doc/commit/b2657c592606c477cd3af256d4a8cd86c38317db

lf- commented 11 months ago

and fixed in nixpkgs, with a bonus fix of the macOS build that got broken for incredibly ????? reasons that had nothing to do with us