standard library: bring the tests into the main CI by amtoine · Pull Request #8525 · nushell/nushell

amtoine · 2023-03-19T10:04:19Z

Should close one of the tasks in #8450.

Description

Note
in order of appearance in the global diff

1b7497c adds the std-tests job to the CI which
1. installs nushell in the runner
2. run the tests.nu module

see open .github/workflows/ci.yml | get jobs.std-tests | to yaml

ec85b6fd..9c122115 is where all the magic happens => see below
🧪 799c7eb introduces some bugs and failing test to see how the CI behaves => see how the tests failed as expected ❌
🧪 and c3de1fa reverts the failing tests, i.e. the previous commit, leaving a standard library whose tests all pass 🎉 => see the tests passing now ✔️

the changes to the runner

see ec85b6fd..9c122115

the issue with the previous runner was the following: the clever trick of using nu -c "use ...; test" did print the errors when occuring but they did not capture the true "failure", i.e. in all cases the $env.LAST_EXIT_CODE was set to 0, never stopping the CI when a test failed 🤔

i first tried to try / catch the error in ec85b6f which kinda worked but only throw a single error, the first one

i thought it was not the best and started thinking about a solution to have a complete report of all failing tests, at once, to avoid running the CI multiple times!

the easiest solution i found was the one i implemented in 9c12211

Warning
this changes the structure of the runner quite a bit, but the for loops where annoying to manipulate structured data and allow the runner to draw a complete report...

now the runner does the following

compute the list of all available tests in a table with the file, module and name columns (first part of the pipe until flatten and rename)
run the tests one by one computing the new pass column
- with a log info
- captures the failing ones => puts true in pass if the test passes, false otherwise
if at least one test has failed, throw a single error with the list of failing tests

hope you'll like it 😌

User-Facing Changes

$nothing

Tests + Formatting

the standard tests now return a true error that will stop the CI

After Submitting

$nothing

this allows the test runner to stop whenever there is a failing test and throw an `$env.LAST_EXIT_CODE` to `1`.

This commit introduces a bug in: - `test_logger::test_criticalansireset` - `test_logger::test_debugansireset` - `test_logger::test_erroransireset` - `test_std::test_assertansireset` - `test_std::test_path_addansireset`

This reverts commit 799c7eb.

codecov · 2023-03-19T10:27:20Z

Codecov Report

Merging #8525 (3f3b9ca) into main (616f065) will increase coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8525      +/-   ##
==========================================
+ Coverage   68.50%   68.51%   +0.01%     
==========================================
  Files         624      631       +7     
  Lines      100672   101962    +1290     
==========================================
+ Hits        68965    69864     +899     
- Misses      31707    32098     +391

see 53 files with indirect coverage changes

amtoine · 2023-03-19T10:32:48Z

mm there is a failing test on macos there which does not happen on linux 🤔

i think this is quite unrelated to this very PR, but still, i will need some help here to fix this 😋

fdncred · 2023-03-19T12:57:51Z

Yup, i get this error on macos too

amtoine · 2023-03-19T13:02:41Z

Yup, i get this error on macos too

mm 🤔

i might be able to have access to a mac but only from tomorrow and at work 😅

if someone wants to try, the best would be to run test_dirs::test_dirs_command manually and show me the content of the failing test maybe? 😌 😇

sholderbach

Love it to have testing set up already!

sholderbach · 2023-03-19T13:04:07Z

.github/workflows/ci.yml

+    steps:
+      - uses: actions/checkout@v3
+
+      - name: Setup Rust toolchain and cache
+        uses: actions-rust-lang/setup-rust-toolchain@v1.4.3
+
+      - name: Install nushell
+        run: cargo install --path . ${{ matrix.flags }}


Could we reuse the virtualenv runner that already has this setup to save some time?

oh that would be super nice 👍

i can have a look after lunch.

ooh do you mean getting rid of the std-tests and simply put the nu .../tests.nu inside python-virtualenv?

ooh do you mean getting rid of the std-tests and simply put the nu .../tests.nu inside python-virtualenv?

Yeah we could combine them. It is a bit worse from a reporting perspective but we could make the std tests the first steps and folllow up with the virtualenv stuff.

maybe we could reuse the artifact, i.e. the nu binary executable, generated by

- name: Install Nushell run: cargo install --locked --path=. --profile ci --no-default-features

in jobs.python-virtualenv ? 🤔

shoud i move nu tests.nu inside python-virtualenv or the other way around? 😋

I would recommend changing the name to something that reflects both. I think you can reuse the --profile ci build, which is debug without internal symbols which saves some time and cache.

a name that reflects both, gotcha 👍

@sholderbach let me know if 38ae95a is ok

mm that did not appear to trigger the CI 🤔

sholderbach · 2023-03-19T13:04:53Z

.github/workflows/ci.yml

+          - style: dataframe
+            flags: "--features=dataframe"


Mhh that might be a limitation on the virtualenv front as we don't run dataframe their anymore.

to be honest with you, i did not really look at the whole CI and just copied the nu-tests job and modified the minimum, i.e. the name, the cargo install above and the nu .../tests.nu 😕

maybe that's a change to apply on the whole CI then ? 😮

I think we should bite the bullet and add it for the ubuntu runner if necessary.

i'm not sure i understand what should be done here 😮

fdncred · 2023-03-19T13:09:25Z

if someone wants to try, the best would be to run test_dirs::test_dirs_command manually and show me the content of the failing test maybe? 😌 😇

test_dirs test_dirs_command 
Error:
  × list is just pwd after initialization
    ╭─[crates/nu-utils/standard_library/test_dirs.nu:24:1]
 24 │         assert (1 == ($env.DIRS_LIST | length)) "list is just pwd after initialization"
 25 │         assert ($base_path == $env.DIRS_LIST.0) "list is just pwd after initialization"
    ·                ────────────────┬───────────────
    ·                                ╰── It is not true.
 26 │
    ╰────

amtoine · 2023-03-19T13:10:07Z

@fdncred
i meant running the individual commands manually, my bad 😌

fdncred · 2023-03-19T13:14:22Z

I can't really run them one at a time because after i cd $base_path I can't do the use statements.

    let base_path = (($nu.temp-path) | path join $"test_dirs_(random uuid)" | path expand )
    let path_a = ($base_path | path join "a")
    let path_b = ($base_path | path join "b")

    #try {
        mkdir $base_path $path_a $path_b
        cd $base_path
        use std.nu "dirs next"
        use std.nu "dirs prev"
        use std.nu "dirs add"
        use std.nu "dirs drop"
        use std.nu "dirs show"

maybe i need to do more setup

fdncred · 2023-03-19T13:54:21Z

After I use them like this

use /Users/fdncred/src/forks/nushell/crates/nu-utils/standard_library/std.nu "dirs show"

I tried this

assert (1 == ($env.DIRS_LIST | length)) "list is just pwd after initialization"
Error: nu::shell::external_command

  × External command failed
   ╭─[entry #63:1:1]
 1 │ assert (1 == ($env.DIRS_LIST | length)) "list is just pwd after initialization"
   ·        ────────────────┬───────────────
   ·                        ╰── Cannot convert bool to a string
   ╰────
  help: All arguments to an external command need to be string-compatible

but if I do

std assert (1 == ($env.DIRS_LIST | length)) "list is just pwd after initialization"

it works - maybe i'm not using something right

yup - i missed the use std.nu assert.

fdncred · 2023-03-19T14:00:33Z

ok, sorry for so many messages here. This is what's going on.

> assert ($base_path == $env.DIRS_LIST.0) "list is just pwd after initialization"
Error:
  × list is just pwd after initialization
   ╭─[entry #76:1:1]
 1 │ assert ($base_path == $env.DIRS_LIST.0) "list is just pwd after initialization"
   ·        ────────────────┬───────────────
   ·                        ╰── It is not true.
   ╰────


> $base_path
/var/folders/m1/x0gp9jy51s146ttxtz7jlpxh0000gn/T/test_dirs_14f52f6b-23fd-4324-a29e-b7f7f0ca314d
> $env.DIRS_LIST.0
/private/var/folders/m1/x0gp9jy51s146ttxtz7jlpxh0000gn/T/test_dirs_14f52f6b-23fd-4324-a29e-b7f7f0ca314d

/tmp is a symlink

^ls -al /tmp
lrwxr-xr-x 1 root wheel 11 Feb  9 03:39 /tmp -> private/tmp

amtoine · 2023-03-19T14:14:44Z

thanks @fdncred for your investigations ❤️

fdncred · 2023-03-19T14:16:32Z

When assert fails, it may be helpful to print out the left hand side and the right hand side so this type of debugging isn't needed.

amtoine · 2023-03-19T14:17:38Z

When assert fails, it may be helpful to print out the left hand side and the right hand side so this type of debugging isn't needed.

agreed

let me address these two issues

amtoine · 2023-03-19T14:46:06Z

#8528 will have to be addressed before this can land then.

i've put it in #8450 👍

presidento · 2023-03-20T06:20:45Z

When assert fails, it may be helpful to print out the left hand side and the right hand side so this type of debugging isn't needed.

It is fixed in #8515

sholderbach · 2023-03-20T19:22:26Z

Apparently there is a merge conflict?

amtoine · 2023-03-21T17:03:02Z

Apparently there is a merge conflict?

yes it's a conflict with the new tests.nu from #8499.

as i know what's wrong, i wanted to resolve the conflicts at the end, after the CI has a good shape 😋

Related to #8525. # Description this should close #8528. # User-Facing Changes ``` $nothing ``` # Tests + Formatting ``` $nothing ``` # After Submitting ``` $nothing ```

presidento · 2023-03-24T07:50:06Z

I tried it, I like the summary at the end.

amtoine · 2023-03-24T13:48:30Z

I tried it, I like the summary at the end.

glad you like it 😌

in order for this to work, i found the following procedure - group by module name - tranpose to get out of the record - iterate over the modules and log in INFO mode - iterate over the module tests and log in DEBUG mode - still construct the same structure by adding `pass` to all test rows and then flattening the whole data

This was causing a "stack overflow" issue in the stdlib execution on windows.

amtoine · 2023-03-24T17:41:08Z

ooook, so after a looooot of debugging and investigation, i found the culprit... 🥁 🥁 🥁

it was the --profile ci in the std-lib-and-python-virtualenv job of the CI 😮

i removed it in 00b820d.

i think i'll address the point made by @presidento in another PR 😌

fdncred · 2023-03-24T17:59:59Z

.github/workflows/ci.yml


      - name: Install Nushell
-        run: cargo install --locked --path=. --profile ci --no-default-features
+        run: cargo install --path . --locked --no-default-features


In an effort to not repeat this problem in the future, would you mind writing up some comments as to why we removed --profile ci? like, it causes stack overflows when running scripts with nu -c, or something like that.

oh yeah for sure 👍

should be good in c0f363a

presidento · 2023-03-24T18:03:35Z

Good job, thank you for your hard work debugging it. I really like the result. I suggest only two small changes:

diff --git a/crates/nu-utils/standard_library/tests.nu b/crates/nu-utils/standard_library/tests.nu
index df72db384..f316d8df7 100644
--- a/crates/nu-utils/standard_library/tests.nu
+++ b/crates/nu-utils/standard_library/tests.nu
@@ -27,7 +27,7 @@ def main [
     --path: path, # Path to look for tests. Default: directory of this file.
     --module: string, # Module to run tests. Default: all test modules found.
     --command: string, # Test command to run. Default: all test command found in the files.
-    --list, # list the tests selected by `--module` and `--command` without running them.
+    --list, # list the selected tests without running them.
 ] {
     let tests = (
         ls ($path | default $env.FILE_PWD | path join "test_*.nu")
@@ -82,11 +82,6 @@ def main [
             ""
         ] | str join "\n")

-        error make {
-            msg: $"(ansi red)std::tests::some_tests_failed(ansi reset)"
-            label: {
-                text: $text
-            }
-        }
+        error make --unspanned { msg: $text }
     }
 }

It is not necessary to mention --module and --command, they may be not used, and there can be other filters as well (by now only the --path parameter).

The second one removes the unnecessary label, just list the errors.

One more small suggestion: Rust may use module :: method notation, but Nushell uses module method. I used that before, so if test_asserts test_assert fails, I can almost copy-paste to run directly use test_asserts.nu; test_asserts test_assert.

These are just suggestions, I accept this PR as it is.

fdncred · 2023-03-24T18:18:17Z

I'm not quite sure I understand but I don't think we should be using std::tests::some_tests_failed terminology because it gives the impression that you can call things this way, which you currently cannot.

These two options are not required to selected the list of tests to run and more options might be added in the future, which would make the documentation quickly deprecated.

amtoine · 2023-03-25T08:45:27Z

thanks for the feedback and the improvement ideas, very sensible 👍

tell me if c0f363aacf7a494b9d954d41b02dafb49c5cfc76..f0f49e081c09c40c692a369b03a9957acdb10e67 fixes all of them 😌
this is the collection of the four last commits.

presidento · 2023-03-25T12:21:27Z

crates/nu-utils/standard_library/tests.nu

-# - "<indentation> x <module>::<test>" all in red if failed
-# - "<indentation>   <module>::<test>" all in green if passed
+# - "<indentation> x <module>  <test>" all in red if failed
+# - "<indentation>   <module>  <test>" all in green if passed


You used double spaces instead of one. If it is intentional, I have no problem with that.

to be honest. it's not on purpose 👀

fixed with the proper <mod> <cmd> notation in 3f3b9ca 😋

presidento · 2023-03-25T12:22:40Z

Perfect. 👌 Thank you for your hard work. 👏

fdncred · 2023-03-25T16:02:09Z

are we ready to land this?

presidento · 2023-03-25T16:26:18Z

I think yes.

amtoine · 2023-03-25T18:28:22Z

are we ready to land this?

👍

@presidento

the first part of this PR comes from a request from @presidento in #8525. the second one is an improvement of the error support. # Description this PR - computes `module_search_pattern` to only `ls` the selected modules => the goal is to save search time in the future with more tests - gives better errors when - the `--path` is invalid - the `--module` does not exist - the search is too strict ### examples ```bash >_ nu crates/nu-utils/standard_library/tests.nu --path does-not-exist Error: × directory_not_found ╭─[<commandline>:1:1] 1 │ main --path does-not-exist · ───────┬────── · ╰── no such directory ╰──── ``` ```bash >_ nu crates/nu-utils/standard_library/tests.nu --module does-not-exist Error: × module_not_found ╭─[<commandline>:1:1] 1 │ main --module does-not-exist · ───────┬────── · ╰── no such module in /home/amtoine/.local/share/git/store/github.com/amtoine/nushell/crates/nu-utils/standard_library/ ╰──── ``` ```bash >_ nu crates/nu-utils/standard_library/tests.nu --command does_not_exist Error: × no test to run ``` instead of the previous ```bash >_ nu crates/nu-utils/standard_library/tests.nu --path does-not-exist Error: × No matches found for /home/amtoine/.local/share/git/store/github.com/amtoine/nushell/does-not-exist/test_*.nu ╭─[/home/amtoine/.local/share/git/store/github.com/amtoine/nushell/crates/nu-utils/standard_library/tests.nu:32:1] 32 │ let tests = ( 33 │ ls ($path | default $env.FILE_PWD | path join "test_*.nu") · ───────────────────────────┬─────────────────────────── · ╰── Pattern, file or folder not found 34 │ | each {|row| {file: $row.name name: ($row.name | path parse | get stem)}} ╰──── help: no matches found ``` ```bash >_ nu crates/nu-utils/standard_library/tests.nu --module does-not-exist Error: × expected table from pipeline ╭─[/home/amtoine/.local/share/git/store/github.com/amtoine/nushell/crates/nu-utils/standard_library/tests.nu:59:1] 59 │ $tests_to_run 60 │ | group-by module · ────┬─── · ╰── requires a table input 61 │ | transpose name tests ╰──── ``` ```bash >_ nu crates/nu-utils/standard_library/tests.nu --command does-not-exist Error: × expected table from pipeline ╭─[/home/amtoine/.local/share/git/store/github.com/amtoine/nushell/crates/nu-utils/standard_library/tests.nu:59:1] 59 │ $tests_to_run 60 │ | group-by module · ────┬─── · ╰── requires a table input 61 │ | transpose name tests ╰──── ``` # User-Facing Changes ``` $nothing ``` # Tests + Formatting ``` $nothing ``` # After Submitting ``` $nothing ```

…olkit (#8629) Related to #8525. # Description With #8525, the tests for the standard library have been enabled in the CI. The only places where these tests are missing are - the PR template - the toolkit This PR adds - `cargo run -- crates/nu-utils/standard_library/tests.nu` to the PR template - the same command as `toolkit test stdlib` - the `test stdlib` command to `toolkit check pr` # User-Facing Changes ``` $nothing ``` # Tests + Formatting the new output of `toolkit check pr`, with the same color code: > - 🟢 `toolkit fmt` > - 🟢 `toolkit clippy` > - 🟢 `toolkit test` > - 🟢 `toolkit test stdlib` # After Submitting ``` $nothing ```

amtoine added 4 commits March 19, 2023 10:50

catch any test error and throw an error

ec85b6f

this allows the test runner to stop whenever there is a failing test and throw an `$env.LAST_EXIT_CODE` to `1`.

rewrite the runner in a structured way

9c12211

add a clone of nu-tests to run the standard library tests

1b7497c

introduce failing tests to see the CI failing

799c7eb

This commit introduces a bug in: - `test_logger::test_criticalansireset` - `test_logger::test_debugansireset` - `test_logger::test_erroransireset` - `test_std::test_assertansireset` - `test_std::test_path_addansireset`

amtoine mentioned this pull request Mar 19, 2023

standard library: a roadmap #8450

Open

7 tasks

Revert "introduce failing tests to see the CI failing"

c3de1fa

This reverts commit 799c7eb.

fdncred added the A:std-library Defining and improving the standard library written in Nu label Mar 19, 2023

sholderbach reviewed Mar 19, 2023

View reviewed changes

amtoine mentioned this pull request Mar 19, 2023

std::tests::test_dirs::test_dirs_command fails on MacOS #8528

Closed

move the stdlib tests to python-virtualenv and merge names

38ae95a

amtoine closed this Mar 20, 2023

amtoine reopened this Mar 20, 2023

amtoine mentioned this pull request Mar 21, 2023

FIX: expand all the base_paths in std::test_dirs #8552

Merged

amtoine added 2 commits March 24, 2023 14:50

list only the tests selected by --command and --module

443959e

This comment was marked as outdated.

Sign in to view

remove the CI profile from cargo install

00b820d

This was causing a "stack overflow" issue in the stdlib execution on windows.

amtoine force-pushed the feature/ci/bring-standard-library-in-and-throw-true-error branch from 37546cf to 00b820d Compare March 24, 2023 17:38

fdncred reviewed Mar 24, 2023

View reviewed changes

amtoine added 5 commits March 25, 2023 09:33

add a little note about the --profile ci removal in the CI

c0f363a

do not mention --module and --command in --list

1460e97

These two options are not required to selected the list of tests to run and more options might be added in the future, which would make the documentation quickly deprecated.

throw a simple unspanned error with only the test report

9c58024

use the nushell " " notation instead of rust's "::" notation

32cc0d7

reduce the indentation in the error report

f0f49e0

presidento reviewed Mar 25, 2023

View reviewed changes

use a single space in the <mod> <cmd> format

3f3b9ca

amtoine mentioned this pull request Mar 25, 2023

REFACTOR: remove the help commands #8611

Closed

sholderbach merged commit 0567407 into nushell:main Mar 25, 2023

amtoine deleted the feature/ci/bring-standard-library-in-and-throw-true-error branch March 25, 2023 18:29

amtoine mentioned this pull request Mar 26, 2023

stdlib: optimize test search and add better errors #8626

Merged

amtoine mentioned this pull request Mar 26, 2023

feature: add the standard library tests to the PR template and the toolkit #8629

Merged

Conversation

amtoine commented Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

the changes to the runner

hope you'll like it 😌

User-Facing Changes

Tests + Formatting

After Submitting

Uh oh!

codecov bot commented Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

amtoine commented Mar 19, 2023

Uh oh!

fdncred commented Mar 19, 2023

Uh oh!

amtoine commented Mar 19, 2023

Uh oh!

sholderbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fdncred commented Mar 19, 2023

Uh oh!

amtoine commented Mar 19, 2023

Uh oh!

fdncred commented Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fdncred commented Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fdncred commented Mar 19, 2023

Uh oh!

amtoine commented Mar 19, 2023

Uh oh!

fdncred commented Mar 19, 2023

Uh oh!

amtoine commented Mar 19, 2023

Uh oh!

amtoine commented Mar 19, 2023

Uh oh!

presidento commented Mar 20, 2023

Uh oh!

sholderbach commented Mar 20, 2023

Uh oh!

amtoine commented Mar 21, 2023

Uh oh!

presidento commented Mar 24, 2023

Uh oh!

amtoine commented Mar 24, 2023

amtoine commented Mar 19, 2023 •

edited

Loading

codecov bot commented Mar 19, 2023 •

edited

Loading

fdncred commented Mar 19, 2023 •

edited

Loading

fdncred commented Mar 19, 2023 •

edited

Loading