dsherret / dax

Cross-platform shell tools for Deno and Node.js inspired by zx.
MIT License
1.05k stars 35 forks source link

progress test fail #57

Closed sigmaSd closed 1 year ago

sigmaSd commented 1 year ago

I tested already the progress api and it works as expected

I tried to checkout the repo, but from some reason this test fails https://github.com/dsherret/dax/blob/main/mod.test.ts#L881

progress => ./mod.test.ts:881:6
error: AssertionError: Values are not equal:

    [Diff] Actual / Expected

    [
+     "Downloading Test",
    ]

  throw new AssertionError(message);
        ^
    at assertEquals (https://deno.land/std@0.167.0/testing/asserts.ts:190:9)
    at file:///home/mrcool/dev/deno/others/dax/mod.test.ts:886:3

I followed with a debugger and it seems for some reason the instantiation seems to fail here https://github.com/dsherret/dax/blob/main/src/console/utils.ts#L166 , so its weird I get the assertion error then, how did it reach there , maybe assertEquals has some internal try catch

I made sure I regenerated the wasm file with deno task wasmbuild, but same test error

dsherret commented 1 year ago

What version of Deno are you running the tests with?

sigmaSd commented 1 year ago
linux
deno 1.29.1+7ce2b58 (canary, x86_64-unknown-linux-gnu)
v8 10.9.194.5
typescript 4.9.4
dsherret commented 1 year ago

Also, what's the full call stack at src/console/utils.ts#L166?

sigmaSd commented 1 year ago

I added log(new Error().stack!;) there

Error
    at getStaticText (file:///home/mrcool/dev/deno/others/dax/src/console/utils.ts:166:17)
    at refresh (file:///home/mrcool/dev/deno/others/dax/src/console/logger.ts:19:28)
    at Object.setItems (file:///home/mrcool/dev/deno/others/dax/src/console/logger.ts:15:10)
    at forceRender (file:///home/mrcool/dev/deno/others/dax/src/console/progress/interval.ts:52:10)
    at ProgressBar.forceRender (file:///home/mrcool/dev/deno/others/dax/src/console/progress/mod.ts:132:5)
    at file:///home/mrcool/dev/deno/others/dax/mod.test.ts:885:6
    at testStepSanitizer (deno:cli/js/40_testing.js:448:13)
    at asyncOpSanitizer (deno:cli/js/40_testing.js:147:15)
    at resourceSanitizer (deno:cli/js/40_testing.js:374:13)
    at Object.exitSanitizer [as fn] (deno:cli/js/40_testing.js:431:15)
dsherret commented 1 year ago

Ok thanks! I see some async code that's not being awaited. I will look into it.

dsherret commented 1 year ago

That makes no sense it ends up there because forceRender will exit if !isInteractiveConsole. Maybe the debugger makes stdin a tty somehow so that changes the behaviour. How are you running the tests?

dsherret commented 1 year ago

Oh, maybe I need to check that stderr is a tty as well. That will probably fix it. I'll open a PR

sigmaSd commented 1 year ago

I tested it and that fixes it

sigmaSd commented 1 year ago

just one more javascript question, so this doesn't need to be awaited https://github.com/dsherret/dax/blob/043be643d7a686971e33f5a3b9f7a84355445c86/src/console/progress/interval.ts#L52 ? even if it internally returns an async function (refresh) https://github.com/dsherret/dax/blob/043be643d7a686971e33f5a3b9f7a84355445c86/src/console/logger.ts#L18

dsherret commented 1 year ago

Yeah, that's what I meant by "I see some async code that's not being awaited". It should have been awaited and forgot to because we don't have floating promise detection in deno lint (kind of annoying and wish we had it). I updated the code now so this issue and those unawaited promises should be fixed.

(I'm going to do a release later tonight probably)

sigmaSd commented 1 year ago

I see, thanks for fixing it so quickly!

because we don't have floating promise detection in deno lint

definitely would be a nice upgrade