Skip to content

[Testing] Test SSH file transfer with images.#208

Merged
JoeZiminski merged 68 commits intomainfrom
test_ssh_with_image
Jul 2, 2025
Merged

[Testing] Test SSH file transfer with images.#208
JoeZiminski merged 68 commits intomainfrom
test_ssh_with_image

Conversation

@JoeZiminski
Copy link
Member

@JoeZiminski JoeZiminski commented Oct 9, 2023

This PR implements SSH tests through a docker image. They key changes in this PR are:

  • update code_test_and_deploy.yml to selectively run SSH tests in linux runners. This is because the Windows and macOS runners already are in a container, so it's not possible to run a container from within these containers ('nested containerisation').
  • Allow SSH transfer from other ports than the default port 22, by setting an environment variable. This was done for tests as port 22 is already used in GitHub actions.This is added in canonical_configs.py. At the moment this is not documented / exposed to users. In general I don't think there will be much need to SSH from other ports except for 22. But it's possible it will be needed.
  • A Dockerfile is added that spins up an ubuntu image with SSH setup so that it can be SSHed into from tests.
  • ssh_test_utils.py now contains all machinery to SSH into the docker image.

Note this required refactoring the test structing, and introducing relative imports into the test suite.

The rest of this PR is pretty much refactoring. base_transfer.py, test_ssh_file_transfer.py, test_ssh_setup.py just contains the already-existing tests. The new additions is testing the SSH through images not through a configuration where paths etc. were entered into the conftest.py. However, any feedback on these tests is of course welcome.

@JoeZiminski JoeZiminski marked this pull request as draft October 10, 2023 10:16
@JoeZiminski JoeZiminski force-pushed the test_ssh_with_image branch 2 times, most recently from 7f9781b to 7d6add8 Compare October 24, 2023 12:15
@JoeZiminski JoeZiminski force-pushed the test_ssh_with_image branch 2 times, most recently from 8d0f0a3 to 9442694 Compare November 10, 2023 09:18
@JoeZiminski JoeZiminski force-pushed the test_ssh_with_image branch from 88591cb to bafb5d0 Compare April 10, 2024 21:44
@JoeZiminski JoeZiminski force-pushed the test_ssh_with_image branch 13 times, most recently from f47884c to b26163b Compare April 22, 2024 17:19
@JoeZiminski JoeZiminski marked this pull request as ready for review April 29, 2024 20:05
@JoeZiminski JoeZiminski requested a review from niksirbi April 29, 2024 20:55
Copy link
Member

@niksirbi niksirbi left a comment

Choose a reason for hiding this comment

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

My intermediate report on this:

  • The ssh tests failed when locally running on MacOS (M2), with docker running
  • The contributing guide should be updated to let contributors know how to handle local ssh tests (either skip them or configure their local machines accordingly).

Copy link
Collaborator

@cs7-shrey cs7-shrey left a comment

Choose a reason for hiding this comment

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

Hey @JoeZiminski I tried the tests in this PR on linux (kali) and windows. With a few changes, the tests work as expected.

I have added some comments.

@JoeZiminski JoeZiminski added this to the v2.8.0 milestone Jun 17, 2025
@JoeZiminski JoeZiminski force-pushed the test_ssh_with_image branch from 50ae318 to c29e250 Compare June 20, 2025 23:19
@JoeZiminski JoeZiminski requested a review from cs7-shrey June 21, 2025 16:57
@JoeZiminski JoeZiminski linked an issue Jun 21, 2025 that may be closed by this pull request
@JoeZiminski JoeZiminski force-pushed the test_ssh_with_image branch from 76b5e80 to 1958363 Compare June 21, 2025 22:28
Copy link
Collaborator

@cs7-shrey cs7-shrey left a comment

Choose a reason for hiding this comment

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

Hey @JoeZiminski, this is really comprehensive and the overall structure is really great. I've added some review comments, please take a look.

Also,

  1. We can delete tests/ssh_test_utils.py as we have the same file now in tests/tests_transfers/ssh
  2. Do consider adding some type hints in the utility functions in base_transfer.py, etc. At some places it was really hard to infer the data flow especially in the parts where the pandas Dataframe is being used.

@JoeZiminski
Copy link
Member Author

Thanks a lot @cs7-shrey for the feedback have made all suggested changes! Let me know what you think

@JoeZiminski JoeZiminski merged commit e32441a into main Jul 2, 2025
24 of 27 checks passed
@JoeZiminski JoeZiminski deleted the test_ssh_with_image branch July 2, 2025 17:13
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.

Continue improving tests

3 participants