Tests: Remove file I/O from all tests that don't need it

[webbedspace]

Related problem

An alarming amount of tests in Nu-command resemble the following:

#[test]
fn removes_duplicate_rows() {
    Playground::setup("uniq_test_1", |dirs, sandbox| {
        sandbox.with_files(vec![FileWithContentToBeTrimmed(
            "los_tres_caballeros.csv",
            r#"
                first_name,last_name,rusty_at,type
                Andrés,Robalino,10/11/2013,A
                Jonathan,Turner,10/12/2013,B
                Yehuda,Katz,10/11/2013,A
                Jonathan,Turner,10/12/2013,B
                Yehuda,Katz,10/11/2013,A
            "#,
        )]);

        let actual = nu!(
            cwd: dirs.test(), pipeline(
            r#"
                open los_tres_caballeros.csv
                | uniq
                | length

            "#
        ));

        assert_eq!(actual.out, "3");
    })
}

All I have to say is: why is file I/O (and CSV parsing) even part of this test?! The test is whether the uniq command works. Just write the input data in Nuon! Adding open and an entire file playground setup just makes the test slower and more difficult to read.

Describe the solution you'd like

Command tests should incisively probe the output of commands using the minimum necessary input to demonstrate whatever behaviour is being tested. Any command that doesn't involve file I/O shouldn't have it included in the test.

Describe alternatives you've considered

No response

Additional context and details

No response

[sholderbach]

Generally yes, the majority of the unittests should focus on primarily exercising the edge cases of the command of interest.
But we also need to make sure we capture the quirks of our engine and nu_protocol. E.g. if the implementation differs for a PipelineData::Liststream and Value::List, directly pumping in the data literals would lose some of the complexity and coverage.
When simplifying the tests the ones describing weird interactions that caused regressions previously, should remain obvious.

[fdncred]

Also, some of these tests have been around longer than Nuon has been around.

[rgwood]

I think it would be good to clean this up where file access is unnecessary, it would simplify our tests.

That said, I don’t think we’ll see any meaningful improvements in test execution time.

[cosineblast]

I'd like to work on this particular issue

[fdncred]

@cosineblast go for it. i've assigned it to you.

[cosineblast]

Is it ok to use | from json to convert tests that describe their input as json, or would it be preferable to use nuon directly in those as well? Example:

#[test]
fn discards_rows_where_given_column_is_empty() {
    let sample_json = r#"
                {
                    "amigos": [
                        {"name":   "Yehuda", "rusty_luck": 1},
                        {"name": "JT", "rusty_luck": 1},
                        {"name":   "Andres", "rusty_luck": 1},
                        {"name":"GorbyPuff"}
                    ]
                }
            "#;

    let actual = nu!(pipeline(&format!(
        "
                '{}' | from json
                | get amigos
                | compact rusty_luck
                | length
            ",
        sample_json
    )));

    assert_eq!(actual.out, "3");
}

[sholderbach]

I think generally, having simpler tests that are focussed on just the properties under test is preferrable...
with the small but nasty caveat that the way PipelineData is used in some commands could permit that behavior is unreachable if the data is e.g. PipelineData::ListStream instead of PIpelineData::Value or vice versa. So a certain diversity in how test data is passed to the command under test can be helpful.

But from json without -o currently uses PipelineData::Value like you get from a literal.

nushell/crates/nu-command/src/formats/from/json.rs

Lines 79 to 80 in f9c1a32

	Ok(convert_string_to_value(string_input, span)?
	.into_pipeline_data_with_metadata(metadata))

So if there is not a specific prior failure mentioned or it makes sense from the context I think this can be turned into a direct literal.