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.

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

#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

merged 3 commits into from
Sep 8, 2023

Conversation

boyvanduuren
Copy link
Contributor

Leverage the existing mock_pages to put an echo batch script on the path and use that as a pager on Windows.

Copy link
Collaborator

@keith-hall keith-hall left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for your contribution

@sharkdp
Copy link
Owner

sharkdp commented Sep 1, 2023

Why did we not see those test failures in CI?

.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

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

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.

@boyvanduuren
Copy link
Contributor Author

@sharkdp good question, quick glance at the pipeline and it seems like everything is ran in bash, also for Windows? That would explain it, seeing as this issue pops up on powershell (or anywhere tests rely on a binary that's not on the path, really).

Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

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

Looks close to being mergeable, just had some more comments.

@@ -2095,6 +2139,7 @@ fn lessopen_and_lessclose_file_piped() {
#[cfg(feature = "lessopen")]
#[test]
#[serial] // Randomly fails otherwise
#[ignore = "randomly failing on some systems"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that this is not enabled by default any longer we can remove this ignore I think

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

} else {
String::from(base)
let mut cmd_and_args = base.split(" ");
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.


out_cmd
}
None => String::from(base),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we want a suffix in this case too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we do :-)

@Enselic
Copy link
Collaborator

Enselic commented Sep 8, 2023

Looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants