Move test harness to separate package

[sonro]

The test utilities, proposed in #58, shouldn't just be in one test suite. The harness can, and should, be used across the project. It could even be used outside of dotenvy entirely.

This PR creates a new workspace member dotenvy_test_util - which contains the test harness along with its documentation.

---

The new test suite itself is nearly finished, see #111.

[allan2]

I can see this being very useful in creating test files, test text, and testing more complicated cases.

I would prefer if the usage of custom assert functions were minimized. Fallible functions such as Efb::add_env_file should return Result. The tests themselves can be written a Result signature as well. A custom error type may be helpful here.

[allan2]

push_str may be more conventional.

[sonro]

If we change to push or append then the other "add" functions should follow suit.

So we'd have:

• push_vars
• push_var
• push_str
• push_strln
• push_bytes
• push_byte

Or the append version.

[allan2]

I think less is better. I think of Efb as a wrapper around a vector of bytes, with one convenience function, add_key_value.

We shouldn't try to do too much by supporting both byte functions and string functions.

My suggestion is to implement some of the Vec functions:
add_byte -> push
add_bytes -> extend_from_slice
add_str -> extend_from_slice(s.as_bytes())
add_strln -> can add a string and push the newline char

And implement the convenience functions:
add_key_value -> add_var
add_vars -> use a for loop with add_var

Test writers can use the Vec API and the string-to-byte conversions they are already familiar with.

[sonro]

I agree, simpler is better. The majority of use cases would be pushing &str, so I do think keeping that as push_str is worthwhile. Therefore, push_bytes makes sense as a corollary.

add_vars does have a performance improvement to looping over add_var. Looping will perform multiple allocations. BUT, I will concede that the majority of new tests will only have 3 vars max in an env file.

So I will remove it.

[allan2]

I'm not a fan of the Builder suffix, since it's too commonly used in the other context, i.e., describing the API design. I would suggest EnvFileAdv, with "Adv" short for Advanced. Perhaps you can think of something better.

[sonro]

Some options:

• EnvFileAdvCreator
• EnvFileMaker
• AdvancedEnvFile

I quite like the last one. The struct can be passed into init_with_env_file and add_env_file, so semantically it makes sense.

[allan2]

I am good with either AdvancedEnvFile or AdvEnvFile. I personally prefer shorter names; they are more adventurous (AdventurousEnvFile?)

[allan2]

Efb is such a good acronym though. EnvFileBuilder has grown on me!

[sonro]

I am currently simplifying this crate. Removing the "default" code I have come to realise this should just be called EnvFileContents and be the primary way one is constructed.

A String, &str, Vec<u8>, &[u8] or EnvFileBuilder, can already be passed to TestEnv::add_env_file. So why not just call this EnvFileContents as it will be the only way of constructing one besides just using the primitive types. Plus that's exactly what it is.

efc is a pretty good acronym too ;)

[sonro]

Fallible functions such as Efb::add_env_file should return Result. The tests themselves can be written a Result signature as well. A custom error type may be helpful here.

Understood, I'll go ahead and add an error type.

You're right for wanting the failed states to be as descriptive as possible. When these utilities were part of the test harness, it made sense to have custom panic messages. Now it is a separate package: errors are far more appropriate.

I would prefer if the usage of custom assert functions were minimized.

Perhaps if we removed the default TestEnv settings the need for custom asserts would be minimized even further. This allows users to set defaults and custom asserts as they see fit.

[allan2]

@sonro I added my comments on Efb above. But if we are adding a safe API, then I am sure this harness will look different.

Would you rather wait for that before updating this PR further?

[sonro]

@sonro I added my comments on Efb above. But if we are adding a safe API, then I am sure this harness will look different.

Would you rather wait for that before updating this PR further?

The new API can still be tested with these utilities too!

#[test]
fn existing_var_no_env_file() {
    let mut testenv = TestEnv::init();
    testenv.add_env_var("TEST_KEY", "test_val");
    test_in_env(&testenv, || {
        let safe_vars = dotenvy::load_all().unwrap(); // or whatever the api ends up being
        let actual = safe_vars.get("TEST_KEY").unwrap();
        assert_eq!("test_val", actual");
    });
}

Plus we need it to thoroughly test the existing API - deprecated or not. We shouldn't mind unsafety within the test harness as it already uses a mutex for thread safety.

[github-actions]