Add regression testing for the custom assets functionality

[Enselic]

Our current CI pipeline does not regression test the custom assets functionality at all. We get a green pipeline even if we replace HighlitingAssets::from_cache with this:

    pub fn from_cache(cache_path: &Path) -> Result<Self> {
        #[allow(unused_variables)]
        let res = Ok(HighlightingAssets::new(
            SerializedSyntaxSet::FromFile(cache_path.join("syntaxes.bin")),
            asset_from_cache(&cache_path.join("themes.bin"), "theme set")?,
        ));

        panic!("does this code run in CI?");

        #[allow(unreachable_code)]
        res
    }

This PR adds code to regression test the custom assets functionality. It can be seen as preparatory work for the verification of #1825, but is of course useful on its own.

[Enselic]

Thanks to both of you for taking a look! This PR is now ready for detailed code review.

[Enselic]

All comments taken care of. Looking forward to another review round.

[sharkdp]

Thank you for the update and the extra test.

If we want developers to run this script locally in their 'dirty' environment, there are some more things that need to be taken care of. We should probably pass --no-config and also remove the BAT_* environment variables that could be relevant. We might actually be able to skip the second part here, as none of them could interfere. See similar code in

bat/tests/integration_tests.rs

Lines 34 to 51 in 9602195

	fn bat_raw_command_with_config() -> Command {
	let mut cmd = Command::cargo_bin("bat").unwrap();
	cmd.current_dir("tests/examples");
	cmd.env_remove("PAGER");
	cmd.env_remove("BAT_PAGER");
	cmd.env_remove("BAT_CONFIG_PATH");
	cmd.env_remove("BAT_STYLE");
	cmd.env_remove("BAT_THEME");
	cmd.env_remove("BAT_TABS");
	cmd.env_remove("BAT_CONFIG_DIR");
	cmd
	}
	fn bat_raw_command() -> Command {
	let mut cmd = bat_raw_command_with_config();
	cmd.arg("--no-config");
	cmd
	}

and

bat/tests/syntax-tests/create_highlighted_versions.py

Lines 11 to 17 in 9602195

	BAT_OPTIONS = [
	"--no-config",
	"--style=plain",
	"--color=always",
	"--theme=default",
	"--italic-text=always",
	]

bat/tests/syntax-tests/create_highlighted_versions.py

Lines 48 to 56 in 9602195

	try:
	env = os.environ.copy()
	env.pop("PAGER", None)
	env.pop("BAT_PAGER", None)
	env.pop("BAT_CONFIG_PATH", None)
	env.pop("BAT_STYLE", None)
	env.pop("BAT_THEME", None)
	env.pop("BAT_TABS", None)
	env["COLORTERM"] = "truecolor" # make sure to output 24bit colors

[Enselic]

Hmm. Not sure I agree it is a good idea to spend time on making the script run in any thinkable environment. There is a difference between "being able to run the script locally" and "making sure the script works, no matter in what environment it is run".

There are many facets to my opinion.

• I consider the CI environment to be the "reference" environment. If a developer wants to run tests, I do not think it is unreasonable to require the developer to make their environment comparable to CI. Or run test in a docker container.
• It will take considerable effort to make the script run in any thinkable environment. Effort that I think is better spent elsewhere.
• It will complicate the code and make it harder to understand and change.

So why did I think it was a good idea to make use of e.g. BAT_CONFIG_DIR and BAT_CACHE_PATH then? Because:

• As with so many things in software development, things are not black and white. I do think it makes sense to prevent casual explorers of the git repo from destructively clearing their cache.
• I think it adds clarity and predictability to the test, without overdoing it.

Admittedly, my opinion might be a bit tainted by my focus on #951. Spending more time on this script would to me be a distraction from #951 rather than help us get #951 fixed quicker. At this point, I just want to get this PR merged, so that I can merge #1825, so that I can update #1787.

[sharkdp]

I consider the CI environment to be the "reference" environment. If a developer wants to run tests, I do not think it is unreasonable to require the developer to make their environment comparable to CI. Or run test in a docker container.

Hm, yes. But as long as we don't provide a clean docker environment for testing out of the box, it's rather inconvenient for developers to modify their local environment to make it "CI-like". It's not unreasonable to have a bat config file as a developer and maybe to have a few BAT_* environment variables locally set.

I didn't add all of this clean up code just because I was thinking of hypothetical scenarios. I added it because tests were locally failing in some environments.

That being said, for this particular script, I don't think we need more clean-up work. And if we missed something, we can also add it in later. It is possible to break the tests (e.g. by setting BAT_OPTS="--no-custom-assets"), but that's a constructed example of course.

It will take considerable effort to make the script run in any thinkable environment. Effort that I think is better spent elsewhere.

Partially agreed. I have spent time in the past figuring out why I couldn't reproduce a bug report. Or a test failure. Time that I could have saved by proactively making sure that the environment is clean/comparable.

For testing, it would be ideal to have a completely side-effect-free program. Somewhat unfortunately, bat has grown to be an application that is quite the opposite. It is very much dependent on its environment (env. variables, terminal properties, less version, OS flavor, config file, …).

Again - not speaking of this PR here, but more generally.

Admittedly, my opinion might be a bit tainted by my focus on #951. Spending more time on this script would to me be a distraction from #951 rather than help us get #951 fixed quicker. At this point, I just want to get this PR merged, so that I can merge #1825, so that I can update #1787.

That's completely fair. I don't want to slow you down. This PR is a great addition for our tests.

[Enselic]

Thanks for being pragmatic despite making valid counter-points 👍

Merging.