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

Call refresh by default in AbstractFileObject.onChange #150

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

boris-petrov
Copy link
Contributor

I think this is a good default. Most providers won't do anything different than this. I guess? @garydgregory thoughts?

@garydgregory
Copy link
Member

Hi @boris-petrov
Run a local build before you create a PR, as this would have show you this change breaks the build.
I am not sure of the semantics, needs studying... You PR would need a new test anyway to validate the change.

@boris-petrov
Copy link
Contributor Author

@garydgregory - I spent a lot of time trying to figure what's going on. I give up. This test failure is definitely a bug in the RamFileProvider but it is difficult to fix. I changed a bunch of things and always something fails.

If you have the time, I would suggest you look into this and try fixing it.

Things that I saw which are not OK:

  1. AbstractFileObject::injectType is only overridden in RamFileObject which seems fishy. I would try to remove this method.

  2. AbstractFileObject::endOutput is a strange method. It is overridden only in RamFileObject which, again, is not nice.

Check also #151. I fixed some more "bad" things in the code around this.

@garydgregory
Copy link
Member

Hi @boris-petrov
Please rebase on git master. TY!

@boris-petrov
Copy link
Contributor Author

@garydgregory - done!

@garydgregory
Copy link
Member

@boris-petrov
Please rebase on master to pickup better unit test assertion messages in ProviderWriteTests.

@boris-petrov
Copy link
Contributor Author

@garydgregory - done!

@garydgregory
Copy link
Member

garydgregory commented Dec 29, 2020

Hi @boris-petrov
As you may have seen in the check list, this PR breaks the build:

Tests run: 93, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 5.007 sec <<< FAILURE! - in org.apache.commons.vfs2.provider.ram.test.RamProviderTestCase
testListener(org.apache.commons.vfs2.test.ProviderWriteTests)  Time elapsed: 0.007 sec  <<< FAILURE!
junit.framework.AssertionFailedError: ram:///write-tests/newfile

Please rebase on master to use the latest but I am thinking that it might be your actual changes that brake the test.

Also this PR does that have a test or tests that fail without your change to the main side of the source tree.

@boris-petrov
Copy link
Contributor Author

@garydgregory - I rebased on master.

Please see this comment. As I said there, the failing test is a real issue in the RAM provider.

Also, please merge #151 before this. It simplifies some code and will help with debugging this issue here.

As for tests, this is not fixing any issues, it's just a convenience for some file systems that won't have to do anything "manually" onChange.

@garydgregory
Copy link
Member

Hi @boris-petrov
May you rebase on master?

@boris-petrov
Copy link
Contributor Author

@garydgregory done!

@garydgregory
Copy link
Member

@boris-petrov
#151 is in, but the builds still fail here. Any ideas?

@boris-petrov
Copy link
Contributor Author

Well, as I've said in a previous comment, there is some bug in the RamFileProvider. I've forgotten what the problem is. I guess someone would need to debug it.

@garydgregory
Copy link
Member

OK, then if someone wants to get this build green, they know where to dig.

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