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

test(scp): Add unit tests for getting remote files #1244

Merged
merged 2 commits into from
Sep 15, 2024

Conversation

yedayak
Copy link
Collaborator

@yedayak yedayak commented Sep 5, 2024

Since there doesn't seem to be any unit tests for xfuncs, I'm not sure if the naming here makes sense, or if I should make some more directories, for example a unit/xfunc or fixtures/xfunc?

I did this since there seems to be a lot of issues with this function, as seen in #910 and #765 (comment)
This should be a first good step to make fixing the issues easier IMO.

@yedayak yedayak force-pushed the scp-remote-unit branch 2 times, most recently from 5322a52 to 1302eb3 Compare September 5, 2024 20:36
@akinomyoga
Copy link
Collaborator

I think this should be tested within test/t/test_scp.py. The test/t/unit/test_unit_*.py are the tests for the core components in bash_completion. Except for that, LGTM.

@yedayak
Copy link
Collaborator Author

yedayak commented Sep 6, 2024

I think this should be tested within test/t/test_scp.py. The test/t/unit/test_unit_*.py are the tests for the core components in bash_completion. Except for that, LGTM.

Moved to test/t/test_scp.py.
I had to add @pytest.mark.bashcomp(ignore_env=r"^\+COMPREPLY=") to the entire class, I'm not sure if there is a better way to only apply it to the necessary functions.

@akinomyoga
Copy link
Collaborator

I'm not sure if there is a better way to only apply it to the necessary functions.

You can call bash_env.save_variable("COMPREPLY").

test/t/test_scp.py Outdated Show resolved Hide resolved
@yedayak
Copy link
Collaborator Author

yedayak commented Sep 7, 2024

I'm not sure if there is a better way to only apply it to the necessary functions.

You can call bash_env.save_variable("COMPREPLY").

This doesn't seem to work, the diff doesn't look at the saved variables, it just uses get_env() which is basically just _comp__test_get_env.

@akinomyoga
Copy link
Collaborator

I'm not sure if there is a better way to only apply it to the necessary functions.

You can call bash_env.save_variable("COMPREPLY").

This doesn't seem to work,

How have you checked that it doesn't seem to work? I now tried that, but it works.

the diff doesn't look at the saved variables, it just uses get_env() which is basically just _comp__test_get_env.

The saved variables are restored by bash_env. get_env() is called after the with statement calls bash_env_saved.__exit__() and the saved variables are restored.

@yedayak
Copy link
Collaborator Author

yedayak commented Sep 7, 2024

I'm not sure if there is a better way to only apply it to the necessary functions.

You can call bash_env.save_variable("COMPREPLY").

This doesn't seem to work,

How have you checked that it doesn't seem to work? I now tried that, but it works.

the diff doesn't look at the saved variables, it just uses get_env() which is basically just _comp__test_get_env.

The saved variables are restored by bash_env. get_env() is called after the with statement calls bash_env_saved.__exit__() and the saved variables are restored.

Ah I managed it, I was trying to save the variable after it was changed, which in hindsight obviously doesn't make sense :)

test/fixtures/scp/bin/ssh Outdated Show resolved Hide resolved
test/t/test_scp.py Outdated Show resolved Hide resolved
test/t/test_scp.py Outdated Show resolved Hide resolved
test/fixtures/scp/bin/ssh Outdated Show resolved Hide resolved
test/fixtures/scp/bin/ssh Show resolved Hide resolved
@yedayak yedayak force-pushed the scp-remote-unit branch 2 times, most recently from 1b29aa5 to 3f355a4 Compare September 11, 2024 17:22
Copy link
Collaborator

@akinomyoga akinomyoga left a comment

Choose a reason for hiding this comment

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

The rest seems good. The mock ssh is of course not a complete implementation of the ssh argument parsing, but we can at least explain its limited behavior.

This tests the current behaviour of the
xfunc_scp_compgen_remote_files function, including escaping, by
introducing a fixture to mock ssh invocation on the local host.
@yedayak yedayak merged commit 0eef733 into scop:main Sep 15, 2024
7 checks passed
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.

2 participants