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

FTP: run tests against (S)FTP server in Docker #1668

Merged
merged 17 commits into from
Apr 30, 2019
Merged

Conversation

tgambet
Copy link
Contributor

@tgambet tgambet commented Apr 26, 2019

Purpose

The purpose of this PR is to move tests from using an embedded FTP and SFTP server to using docker.

References

References #1663

Changes

Test files

All test classes related to setting up the embedded servers have been removed or simplified.
Some files have been renamed to remove the confusion between FPT and SFTP specific files.

There are now only two main classes that tests can extend: BaseFtpSupport for FTP and FTPS support and BaseSftpSupport for SFTP support.

Issues encountered:

  • the SFTP server doesn't allow writing to the user root folder so BaseSftpSpec and java equivalents had to be adapted to run tests inside an upload/ subfolder transparently.
  • the SFTP server does not accept keys that are publicly readable for security reasons, which is an issue for the current use case where the keys are committed to github. To prevent having to chmod the keys every time, I had to mount an init.sh script which overwrites the servers's self-generated keys on each startup and sets the correct rights.
  • writing to the server outputstream and getting an IOResult doesn't guarantee that a file has indeed been fully written to disk yet, especially now that the server doesn't run on the jvm. To make tests pass more consistently I added an eventually clause whenever a file content must be checked.
  • the SFTP server does not allow authentication with an invalid username regardless of whether we are using a key or not so the credentials passed to SftpSettings have to take that into account.
  • testing connection failure by stopping the server is not possible anymore. Might be worth investigating using a killswitch to test correct reporting of failure?

Production files

  • CommonFtpOperations has been changed to take into account the server timezone.
    While testing against docker I found out that there was a two hours discrepancy between the system time and the time reported in the FtpFile (I'm in GMT+2). It turns out that the underlying FTPClient gives a Calendar instance for the last modified time which is subject to timezones. Setting the timezone to UTC fixes the issue but this should be reviewed more thoroughly.
  • FtpIOGraphStage calls completePendingCommand where needed. See FTP: sporadic test failures - toPath does not write all bytes to file #1669.

@tgambet tgambet marked this pull request as ready for review April 27, 2019 11:27
@tgambet
Copy link
Contributor Author

tgambet commented Apr 28, 2019

Relevant Travis logs:

[info] Detected changes in directories: [, ftp, project]
[error] java.lang.RuntimeException: Could not create directory /home/travis/build/akka/alpakka/ftp/target/streams/$global/dependencyPositions/$global/streams

Not sure what's going on but it happens consistently. @ennru could you take a look please?

On my local environment running sbt ftp/testChanged results in a FileNotFoundException as scalafmt is trying to format a deleted file. I gave up trying to debug that. Help would be appreciated.

@ennru
Copy link
Member

ennru commented Apr 28, 2019

I'm not sure about the Travis problem, will have a look tomorrow.
Your local problem should be solved by a clean in ftp.

* work with different file names to fix sporadic tests failures
@tgambet
Copy link
Contributor Author

tgambet commented Apr 28, 2019

Thank you, that did the trick.

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

Great work, this makes me much more confident in the Ftp module's tests.

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

LGTM.

@ennru ennru merged commit afd9821 into akka:master Apr 30, 2019
@ennru ennru added this to the 1.0.1 milestone Apr 30, 2019
@ennru
Copy link
Member

ennru commented Apr 30, 2019

Thank you once more for this effort, I really appreciate working with you.

@tgambet
Copy link
Contributor Author

tgambet commented Apr 30, 2019

Awesome, my pleasure.

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

Successfully merging this pull request may close these issues.

None yet

2 participants