tests failing in core on Windows

@gcliff I meant what is inside the folders. I am assuming the mp3 files created during the test.

I will disable the tests on Windows using @achilep proposal for now. The tests will still run in all our pipelines which ensures the behavior of the code will not change. In the meantime I will try to replicate your environment and the errors you are seeing. So that I can investigate more easily and look for a fix :ok_hand:

@gcliff I pushed @achilep suggestion. So these 2 tests that fail on your machine should now be disabled on Windows.

This will be hard :sweat: So @gcliff my research tells me you run Windows 7, correct? I was able to run the tests inside a Microsoft provided docker image with JDK8 https://hub.docker.com/_/microsoft-java-jdk but I assume that was naive since that is most likely very different from actual Windows since its meant for deploying things to Azure.

for the curious this is what you do

docker pull mcr.microsoft.com/java/maven:8-zulu-debian9
docker run -it --name windows -v ~/.m2:/root/.m2 -v ~/code/openmrs/openmrs-core:/code --workdir /code mcr.microsoft.com/java/maven:8-zulu-debian9 sh
mvn clean test -Dtest=BinaryStreamHandlerTest,MediaHandlerTest

I will most likely need to setup a VM. Not sure yet if I can get a Windows 7.

Just to point this out, you’ve actually pulled a Microsoft-provided Debian image, which really doesn’t help with the Windows-specific file handling issue. The minimal Windows image seems to be called 8-zulu-windowsservercore…

They’re generated here in the BaseContextSensitiveTest class which means that we generate probably 200ish directories per Maven build. They should be deleted on exit, but apparently some aren’t. Although I don’t get the exception you’re reporting on my machine (I use a Mac), I do see a number of those appdir-for-unit-tests- directories piled up in my temporary files location. When I clean the temp directory and re-run the tests, I end up with 11 hanging appdir-for-unit-tests- directories.

Here is the full listing of files that aren’t getting cleaned up on exit (for my machine).’

@gcliff If you could get a similar listing for your machine, it might be helpful in tracking down where we have unclosed files.

Thanks at @ibacher, if you remove the @DisabledOnOs(WINDOWS) flag on for example the MediaHandlerTest does it fail telling you the Jupiter Extension could not delete the temp dir because some other process was holding on to things in there (like in @gcliff logs)?

I just ran spotbugs on openmrs-core. Both tests that fail on Windows have a Bug descriptions — spotbugs 4.7.1 documentation on the getObs() method. They are both creating an InputStream pass them on to the ComplexData.data and never close it. See here for example openmrs-core/MediaHandler.java at master · openmrs/openmrs-core · GitHub

I just pushed TRUNK-5816 Attempt fix failing HandlerTests on Windows · openmrs/openmrs-core@6dc1a74 · GitHub in an attempt to close them myself in the test.

Can you @ibacher and @gcliff please run the MediaHandlerTest and BinaryStreamHandlerTest (remove the @DisabledOn) and let me know if that changes anything?

As I said before, I’m running a Mac, so I can’t reproduce the issue locally :slight_smile:. I was simply pointing out that we do seem to be leaving other test-related detritus around.

ah, sorry missed that post.

That is yet another story. Did not know we do that. The issue we are seeing on Windows is that JUnit 5 @TempDir cannot remove the temp dir since some things in our code are still holding on to the files created in these temp dirs. These JUnit temp dirs are not the ones you listed.

@gcliff can you please try my suggestion since I and Ian cannot reproduce your error. You will need to pull the latest changes and remove the @DisabledOn on these 2 failing tests.

Anyone out there developing on Windows that can try the above? I tried all kinds of things to install a Windows VM/container on my machine but failed so far. I cannot reproduce the error @gcliff is seeing on Windows and thus not test if my change fixed it. Thanks a lot!

Good news is I can reproduce what @gcliff is seeing using GitHub actions in openmrs-core :smile: https://github.com/openmrs/openmrs-core/pull/3380/checks#step:4:1935 Bad news is I did not fix the issue with my latest changes to the test :frowning:

Bright side is we can keep the tests disabled on master on Windows so devs on Windows can work. Meanwhile we can work on a branch like https://github.com/openmrs/openmrs-core/pull/3380 where we enable them, make changes and they will be tested on Windows :ok_hand:

Reproducibility is the first step :sweat_smile:

hello @teleivo @ibacher sorry for the delayed response ,i have been offline for the last 24 hours because it was Sabbath . :pray:

1 Like

YES ,windows 7

no worries! I figured out a way to reproduce it :slight_smile:

no the question is only how to solve it :thinking:

thats great ,am doing a build now and going to be sharing soon

sure

unless you changed something, I do not expect it to be different. Since I can now see the error you are seeing when you run these 2 tests on Windows.

1 Like

So,

I can only make the tests pass on Windows if I close the FileInputStream we put into the obs ComplexData.data

changes and see passing tests on Windows GitHub check on Windows

That is obviously not a fix because it means that after doing handler.getObs(obs1, "RAW_VIEW").getComplexData().getData() the FileInputStream is already closed an cannot be read.

Can someone explain to me why closing the streams in the test at https://github.com/openmrs/openmrs-core/pull/3380/commits/a4c7c897cba198146cdf2af0c0182d60ab061351#diff-1dfc7fecf17fefaff8bfde15c13b002bR113-R114 does not actually make the test pass? I did try to close it before and then on these lines I got, Stream is closed Exception.

Is it something super obvious that I am not seeing because I am tired :sweat_smile: I am unsure about this API. The data is of type Object how does anyone know that it is a stream or when it is a stream? What type of stream? And that its already open? And that one should make sure to close it.

What are our options? Any ideas. I am not sure what to do with these tests or the behaviour. I do think the JUnit 5 implementation of @TempDir seems more strict, which is good as it can uncover such things.

cc @dkayiwa @wyclif @mseaton @ibacher kindly requesting for your thoughts on this …

Thanks

1 Like

Can you create a ticket for it?

1 Like

cool: https://issues.openmrs.org/browse/TRUNK-5876