Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: collect the output of gm repo compare in the tests #198

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amtoine
Copy link
Owner

@amtoine amtoine commented Jul 19, 2024

related to

running tk test compare --verbose would give the following error:

Error:   × invalid error format.
    ╭─[NU_STDLIB_VIRTUAL_DIR/std/assert.nu:44:16]
 43 │             msg: ($message | default "Assertion failed."),
 44 │ ╭─▶         label: ($error_label | default {
 45 │ │               text: "It is not true.",
 46 │ │               span: (metadata $condition).span,
 47 │ ├─▶         })
    · ╰──── `$.label.start` should be smaller than `$.label.end`
 48 │         }
    ╰────
  help: 112592 > 104293

it appears the gm repo branch compare command outputs a "byte stream" instead of a string 🤔
what i don't understand:

  • where this error comes from
  • why the get-commit test passes without collect, because gm repo get commit also returns a byte stream instead of a string

@amtoine amtoine added the fix A fix for a bug label Jul 19, 2024
@devyn
Copy link

devyn commented Jul 19, 2024

Byte streams are the normal output of external commands. They can be used as strings as long as they parse as UTF-8, and they can also be used as binary. Having a byte stream isn't unusual, and you might also see string (stream) which can only be used as a string, but generally is produced by commands that definitely make good UTF-8 data (so internal commands that make potentially big strings).

Since the culprit is the value of $condition, can you tell me a little bit more about what the argument to assert might be? It would help to be able to narrow this down to the test that's producing it.

@amtoine
Copy link
Owner Author

amtoine commented Jul 19, 2024

well, i can't reproduce this for now 🤔

if i add a few debug prints to the test to extract the values of actual and expected

diff --git a/pkgs/nu-git-manager-sugar/tests/git.nu b/pkgs/nu-git-manager-sugar/tests/git.nu
index 9057dfe..df5414e 100644
--- a/pkgs/nu-git-manager-sugar/tests/git.nu
+++ b/pkgs/nu-git-manager-sugar/tests/git.nu
@@ -356,6 +356,12 @@ export def branch-compare [] {
         "\\ No newline at end of file"
         "",
     ]
+    print "ACTUAL---"
+    print (gm repo compare main)
+    print "---END"
+    print "EXPECTED"
+    print ($expected | str join "\n")
+    print "---END"
     assert equal (gm repo compare main) ($expected | str join "\n")
     assert equal (gm repo compare main --head HEAD) ($expected | str join "\n")
 

i get

let actual = 'diff --git a/foo.txt b/foo.txt
new file mode 100644
index 0000000..1910281
--- /dev/null
+++ b/foo.txt
@@ -0,0 +1 @@
+foo
\ No newline at end of file'

let expected = 'diff --git a/foo.txt b/foo.txt
new file mode 100644
index 0000000..1910281
--- /dev/null
+++ b/foo.txt
@@ -0,0 +1 @@
+foo
\ No newline at end of file
'

but

use std assert
assert equal $actual $expected

does not fail with the "span" issue

idea

maybe that's because actual is a Nushell string there and not an external byte stream.
but if i try a dummy example with an external command, it does not fail

^echo "foo" | describe # shows "byte stream"

but

assert equal (^echo "foo") "foo" # does not fail

@amtoine
Copy link
Owner Author

amtoine commented Jul 19, 2024

also, if would expect metadata to always produce a valid span, which is not the case here 🤔

@fdncred
Copy link

fdncred commented Jul 19, 2024

I thought the CI error was because you are running ubuntu 20.04 in CI versus ubuntu latest.

@amtoine
Copy link
Owner Author

amtoine commented Jul 19, 2024

@fdncred maybe you're referring to the failing CI from nushell/nupm#94? 😋

@fdncred
Copy link

fdncred commented Jul 19, 2024

@amtoine Ya, sorry.

@amtoine
Copy link
Owner Author

amtoine commented Jul 19, 2024

@fdncred aha, no worries, these two CI issues are haunting my dreams 🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix A fix for a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants