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.

Sign up for GitHub

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

Jump to bottom

#2592: Fixes unit tests dependent on echo on windows #2602

Merged
merged 3 commits into from
Sep 8, 2023
Merged

#2592: Fixes unit tests dependent on echo on windows #2602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea668f0
#2592: Fixes unit tests dependent on echo on windows
boyvanduuren Jun 22, 2023
d9869ff
Run cargo test on powershell for windows targets
boyvanduuren Sep 3, 2023
e826727
Improvements after review
rbbcd Sep 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion .github/workflows/CICD.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,13 @@ jobs:

- name: Run tests
shell: bash
run: $BUILD_CMD test --locked --target=${{ matrix.job.target }} ${{ steps.test-options.outputs.CARGO_TEST_OPTIONS}}
run: |
if [[ ${{ matrix.job.os }} = windows-* ]]
then
powershell.exe -command "$BUILD_CMD test --locked --target=${{ matrix.job.target }} ${{ steps.test-options.outputs.CARGO_TEST_OPTIONS}}"
else
$BUILD_CMD test --locked --target=${{ matrix.job.target }} ${{ steps.test-options.outputs.CARGO_TEST_OPTIONS}}
fi

- name: Run bat
shell: bash
Expand Down
5 changes: 5 additions & 0 deletions tests/examples/bat-windows.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Make sure that the pager gets executed
--paging=always

# Output a dummy message for the integration test and system wide config test.
--pager="echo.bat dummy-pager-from-config"
205 changes: 126 additions & 79 deletions tests/integration_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ use utils::mocked_pagers;

const EXAMPLES_DIR: &str = "tests/examples";

fn get_config() -> &'static str {
if cfg!(windows) {
"bat-windows.conf"
} else {
"bat.conf"
}
}

#[test]
fn basic() {
bat()
Expand Down Expand Up @@ -589,37 +597,49 @@ fn do_not_exit_directory() {
}

#[test]
#[serial]
fn pager_basic() {
bat()
.env("PAGER", "echo pager-output")
.arg("--paging=always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("pager-output\n").normalize());
mocked_pagers::with_mocked_versions_of_more_and_most_in_path(|| {
Copy link
Collaborator

@Enselic Enselic Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution. I don't quite follow along here though. Why do we need to change for example this pager_basic() test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're welcome. The reason we're using the mocked versions here, is so that we actually have an echo available for powershell. See my comment on the original bug report: #2592. echo is not resolved, so on powershell tests that rely on it fail.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I never tried to run cargo test in PowerShell on Windows before. But now when I do these tests indeed fail. Any chance you would be willing to look into running our tests in CI in PowerShell on Windows? Some instructions: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-powershell

In our .github\workflows\CICD.yml and matrix: I think we would want to parameterize on shell: so that Windows tests run with shell: pwsh. Unless we continuously run the tests in PowerShell, it will for sure break again without anyone not noticing.

If you are not interested in doing this, that's totally fine. Just let me know, and I will do a more detailed review of this PR as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I don't mind taking a look. Do you want it as part of this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Yes I think that would be good, that way we know the tests pass in CI PowerShell before we merge.

Copy link
Contributor

@Anomalocaridid Anomalocaridid Sep 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Enselic After taking a quick look at the test output and code, I am not entirely sure what the cause is.

Also, strangely, if I only run the integration tests with cargo test --test integration_tests it does not appear to fail on my system. Even when running it repeatedly using chronic from moreutils.

If I had to make an educated guess, it's some sort of data race that's interfering with $LESSCLOSE. Likely by either keeping it from executing or by just preventing its output from reaching the test runner. I am not sure if it is related to the stdin and temp file parts of the test or if it's just a matter of when cargo runs it relative to the other tests.

I'll try to take a closer look later when I get a chance, but I am honestly not even sure how to go from here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't reproduce any flakiness unfortunately. Not in WSL 2 nor in PowerShell.

@boyvanduuren Feel free to add an #[ignore = "failing intermittently on some systems"] in your PR for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All green.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@boyvanduuren Thanks

@Anomalocaridid: if I run that test "manually", I get an error message from less:

> export LESSOPEN="-echo empty.txt"
> export LESSCLOSE="echo lessclose: %s %s"
> echo "test.txt" | bat
LESSOPEN ignored: must contain exactly one %s

Could this be the issue? Maybe the race condition is somehow related to whether or not that error is printed first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sharkdp Possibly? However, it looks like that error also appears when manually running the other tests where $LESSCLOSE has a dash as part of its prefix. But those do not have the same issue, right?

A recent comment on my PR for $LESSOPEN support gave me a helpful lead.

Basically, they said that the files were going through $LESSOPEN twice, through both bat and less. Sure enough, by running the test manually with less instead of bat, the same LESSOPEN ignored message appears. Also it may be worth mentioning that the message is not present in the code for bat's $LESSOPEN implementation.

So for some reason, the $LESSOPEN environment variable is being passed on to less, but only when $LESSOPEN has a dash in it that indicates that stdin is to be preprocessed as well as files.

When I get a chance, I'll try to fix both $LESSOPEN being passed to less as well as make bat more closely match less's behavior for $LESSOPEN without a %s.

bat()
.env("PAGER", mocked_pagers::from("echo pager-output"))
.arg("--paging=always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::str::contains("pager-output\n").normalize());
});
}

#[test]
#[serial]
fn pager_basic_arg() {
bat()
.arg("--pager=echo pager-output")
.arg("--paging=always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("pager-output\n").normalize());
mocked_pagers::with_mocked_versions_of_more_and_most_in_path(|| {
bat()
.arg(format!(
"--pager={}",
mocked_pagers::from("echo pager-output")
))
.arg("--paging=always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::str::contains("pager-output\n").normalize());
});
}

#[test]
#[serial]
fn pager_overwrite() {
bat()
.env("PAGER", "echo other-pager")
.env("BAT_PAGER", "echo pager-output")
.arg("--paging=always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("pager-output\n").normalize());
mocked_pagers::with_mocked_versions_of_more_and_most_in_path(|| {
bat()
.env("PAGER", mocked_pagers::from("echo other-pager"))
.env("BAT_PAGER", mocked_pagers::from("echo pager-output"))
.arg("--paging=always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::str::contains("pager-output\n").normalize());
});
}

#[test]
Expand All @@ -635,55 +655,73 @@ fn pager_disable() {
}

#[test]
#[serial]
fn pager_arg_override_env_withconfig() {
bat_with_config()
.env("BAT_CONFIG_PATH", "bat.conf")
.env("PAGER", "echo another-pager")
.env("BAT_PAGER", "echo other-pager")
.arg("--pager=echo pager-output")
.arg("--paging=always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("pager-output\n").normalize());
mocked_pagers::with_mocked_versions_of_more_and_most_in_path(|| {
bat_with_config()
.env("BAT_CONFIG_PATH", get_config())
.env("PAGER", mocked_pagers::from("echo another-pager"))
.env("BAT_PAGER", mocked_pagers::from("echo other-pager"))
.arg(format!(
"--pager={}",
mocked_pagers::from("echo pager-output")
))
.arg("--paging=always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::str::contains("pager-output\n").normalize());
});
}

#[test]
#[serial]
fn pager_arg_override_env_noconfig() {
bat()
.env("PAGER", "echo another-pager")
.env("BAT_PAGER", "echo other-pager")
.arg("--pager=echo pager-output")
.arg("--paging=always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("pager-output\n").normalize());
mocked_pagers::with_mocked_versions_of_more_and_most_in_path(|| {
bat()
.env("PAGER", mocked_pagers::from("echo another-pager"))
.env("BAT_PAGER", mocked_pagers::from("echo other-pager"))
.arg(format!(
"--pager={}",
mocked_pagers::from("echo pager-output")
))
.arg("--paging=always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::str::contains("pager-output\n").normalize());
});
}

#[test]
#[serial]
fn pager_env_bat_pager_override_config() {
bat_with_config()
.env("BAT_CONFIG_PATH", "bat.conf")
.env("PAGER", "echo other-pager")
.env("BAT_PAGER", "echo pager-output")
.arg("--paging=always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("pager-output\n").normalize());
mocked_pagers::with_mocked_versions_of_more_and_most_in_path(|| {
bat_with_config()
.env("BAT_CONFIG_PATH", get_config())
.env("PAGER", mocked_pagers::from("echo other-pager"))
.env("BAT_PAGER", mocked_pagers::from("echo pager-output"))
.arg("--paging=always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::str::contains("pager-output\n").normalize());
});
}

#[test]
#[serial]
fn pager_env_pager_nooverride_config() {
bat_with_config()
.env("BAT_CONFIG_PATH", "bat.conf")
.env("PAGER", "echo other-pager")
.arg("--paging=always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("dummy-pager-from-config\n").normalize());
mocked_pagers::with_mocked_versions_of_more_and_most_in_path(|| {
bat_with_config()
.env("BAT_CONFIG_PATH", get_config())
.env("PAGER", mocked_pagers::from("echo other-pager"))
.arg("--paging=always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::str::contains("dummy-pager-from-config\n").normalize());
});
}

#[test]
Expand Down Expand Up @@ -809,15 +847,18 @@ fn alias_pager_disable() {
}

#[test]
#[serial]
fn alias_pager_disable_long_overrides_short() {
bat()
.env("PAGER", "echo pager-output")
.arg("-P")
.arg("--paging=always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("pager-output\n").normalize());
mocked_pagers::with_mocked_versions_of_more_and_most_in_path(|| {
bat()
.env("PAGER", mocked_pagers::from("echo pager-output"))
.arg("-P")
.arg("--paging=always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::str::contains("pager-output\n").normalize());
});
}

#[test]
Expand All @@ -844,14 +885,17 @@ fn pager_failed_to_parse() {
}

#[test]
#[serial]
fn env_var_bat_paging() {
bat()
.env("BAT_PAGER", "echo pager-output")
.env("BAT_PAGING", "always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("pager-output\n"));
mocked_pagers::with_mocked_versions_of_more_and_most_in_path(|| {
bat()
.env("BAT_PAGER", mocked_pagers::from("echo pager-output"))
.env("BAT_PAGING", "always")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::str::contains("pager-output\n").normalize());
});
}

#[test]
Expand Down Expand Up @@ -912,13 +956,16 @@ fn config_location_from_bat_config_dir_variable() {
}

#[test]
#[serial]
fn config_read_arguments_from_file() {
bat_with_config()
.env("BAT_CONFIG_PATH", "bat.conf")
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("dummy-pager-from-config\n").normalize());
mocked_pagers::with_mocked_versions_of_more_and_most_in_path(|| {
bat_with_config()
.env("BAT_CONFIG_PATH", get_config())
.arg("test.txt")
.assert()
.success()
.stdout(predicate::str::contains("dummy-pager-from-config\n").normalize());
});
}

#[cfg(unix)]
Expand Down
1 change: 1 addition & 0 deletions tests/mocked-pagers/echo.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ECHO %*
18 changes: 14 additions & 4 deletions tests/utils/mocked_pagers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,16 @@ fn get_mocked_pagers_dir() -> PathBuf {
/// On Unix: 'most' -> 'most'
/// On Windows: 'most' -> 'most.bat'
pub fn from(base: &str) -> String {
if cfg!(windows) {
format!("{}.bat", base)
} else {
String::from(base)
let mut cmd_and_args = shell_words::split(base).unwrap();
let suffix = if cfg!(windows) { ".bat" } else { "" };
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we already depend on shell_words::split I think this code would end up simpler if we used it here too? If it ends up being worse, we can keep this as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, refactored it a bit further as well.

let mut out_cmd = format!("{}{}", cmd_and_args.first().unwrap(), suffix);

if (cmd_and_args.len() > 1) {
out_cmd.push(' ');
out_cmd.push_str(cmd_and_args[1..].to_vec().join(" ").as_str());
}

out_cmd
}

/// Prepends a directory to the PATH environment variable
Expand Down Expand Up @@ -62,6 +67,11 @@ pub fn with_mocked_versions_of_more_and_most_in_path(actual_test: fn()) {
.assert()
.success()
.stdout(predicate::str::contains("I am most"));
Command::new(from("echo"))
.arg("foobar")
.assert()
.success()
.stdout(predicate::str::contains("foobar"));

// Now run the actual test
actual_test();
Expand Down