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

Bulletproof TFTPServerPathTest #173

Merged
merged 4 commits into from
Sep 16, 2023

Conversation

jkbkupczyk
Copy link
Contributor

@jkbkupczyk jkbkupczyk commented Aug 12, 2023

  • Rewrite TFTPServerPathTest
  • Add the TFTPServer::getPort method to return TFTPServer port

This PR tries to get rid of notoriously failing TFTPServerPathTest on Windows 8 on CI pipeline with the message TFTPServerPathTest.testReadOnly:82 Couldn't clear output location, deleted= ==> expected: <false> but was: <true>

FYI
@garydgregory @kinow

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Code looks good, but there might be something else that needs to be done in the new test, CI failing on ubuntu + Java 8

Error:  Failures: 
Error:    TFTPServerPathTest.testWriteOnly:154 expected: <false> but was: <true>

Thank you!

@jkbkupczyk
Copy link
Contributor Author

Code looks good, but there might be something else that needs to be done in the new test, CI failing on ubuntu + Java 8

Error:  Failures: 
Error:    TFTPServerPathTest.testWriteOnly:154 expected: <false> but was: <true>

Thank you!

Thank you @kinow for the fast response!
I removed the logic of verifying the written contents and moved it to another test TFTPServerPathTest::testWriteVerifyContents. I think it should work RN :)

@garydgregory
Copy link
Member

@jkbkupczyk
Would you please rebase on master?

@jkbkupczyk
Copy link
Contributor Author

@jkbkupczyk Would you please rebase on master?

done

@garydgregory garydgregory merged commit 3750563 into apache:master Sep 16, 2023
8 checks passed
asfgit pushed a commit that referenced this pull request Sep 16, 2023
@garydgregory
Copy link
Member

TY @jkbkupczyk !

@jkbkupczyk
Copy link
Contributor Author

Thank you @garydgregory and @kinow for review!

asfgit pushed a commit that referenced this pull request Sep 17, 2023
@garydgregory
Copy link
Member

@jkbkupczyk
Copy link
Contributor Author

@jkbkupczyk Still fails : https://github.com/apache/commons-net/actions/runs/6213336399/job/16864291327

Hm, I think it's because files have the same name, I will create a patch that will address this issue.

@garydgregory
Copy link
Member

@jkbkupczyk Still fails : https://github.com/apache/commons-net/actions/runs/6213336399/job/16864291327

Hm, I think it's because files have the same name, I will create a patch that will address this issue.

TY!

@jkbkupczyk
Copy link
Contributor Author

@jkbkupczyk Still fails : https://github.com/apache/commons-net/actions/runs/6213336399/job/16864291327

Hm, I think it's because files have the same name, I will create a patch that will address this issue.

TY!

I created patch #184

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.

3 participants