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
fix(sipi): Don't expect API v1 status code (DSP-1114) #1763
Conversation
Something very strange is happening in If I do
Now I set a breakpoint at line 305 of I run the test in IntelliJ's debugger. At the breakpoint, I can see that Sipi returned Now I set another breakpoint at line 438 and resume the test. At the breakpoint, the test has uploaded a file and is requesting the newly created file. I copy the IIIF URL ( So:
I can't figure out where the Sipi configuration is coming from in the test. In Any ideas @subotic @SepidehAlassi ? |
There is one small difference. In the tests, the Docker containers are started through the |
So where does the Sipi test container get its configuration files from? I suspect that the Lua pre_flight function isn’t being called at all inside the test container. How can I find out? |
The config and scripts folders are backed into the Docker image. This is why we publish a DSP-API-specific Sipi Docker image. |
you can see this here: dsp-api/docker/knora-sipi/BUILD.bazel Line 11 in 346873d
|
So I guess that means that if I need to change a Sipi script and test it, I need to publish a new Sipi Docker image. How do I do that? |
hmm, something does not look right. The config folder should be added to the Sipi image, and also used when starting the container for the tests. |
you recreate the docker containers locally with |
I did that, and it had no effect on the output that I described above. |
ok, it should not be necessary to recreate the docker containers. I implemented it better:
|
The sipi scripts are mounted into the image before the container is started. They are put on the class path. |
Line 140 in 346873d
|
or more correctly, they are part of the "it_library" which is a dependency for all it tests: Line 199 in 346873d
|
But in that case, Sipi should print the following when run inside the test container:
But it doesn't. That's what makes me think that the |
@benjamingeer I found the error. I made a mistake when "mounting" the |
@subotic No worries, I'm glad it turns out to be easy to fix. :) |
@subotic But now I get this error in the Sipi log when I do
|
If you use Bazel directly, then you first need to manually publish the new Docker image locally: |
did you run |
I've just done:
I stopped the Ryuk container as soon as it appeared. The test failed with:
The log in the Sipi test container says:
|
Should it be using |
The very strange thing is that if I comment out the test |
It should actually be your machine's IP address. |
Is there something I need to configure to fix that? Sipi says it's using |
@@ -27,6 +28,10 @@ import org.testcontainers.containers.{BindMode, GenericContainer} | |||
*/ | |||
object TestContainers { | |||
|
|||
// get local IP address, which we need for SIPI | |||
val localhost: InetAddress = InetAddress.getLocalHost | |||
val localIpAddress: String = localhost.getHostAddress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be your machine's IP. Could you maybe run it in debug mode and see what you get here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a static initialiser bug: if so, these val
s need to be lazy
because otherwise they might not be initialised before they're needed. Will confirm after lunch.
@@ -35,14 +40,12 @@ object TestContainers { | |||
|
|||
val SipiContainer = new GenericContainer("bazel/docker/knora-sipi:image") | |||
SipiContainer.withExposedPorts(1024) | |||
SipiContainer.withEnv("SIPI_WEBAPI_HOSTNAME", "api") | |||
SipiContainer.withEnv("SIPI_WEBAPI_HOSTNAME", localIpAddress) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here your machine's IP should be used to configure Sipi.
|
This is what I get. Also, I see that you have Java 14 installed. |
I've pushed the fix for the testcontainer deprecation |
Maybe Homebrew installed it with Scala. The only JVM I've installed is AdoptOpenJDK 11:
|
public class Test {
public static void main(String[] args) throws Exception {
System.out.println(java.net.InetAddress.getLocalHost().getHostAddress());
}
}
|
Maybe this is the solution: https://stackoverflow.com/a/16444863 |
OK I found the problem! It actually was a static initialisation problem, but not the kind I expected. The problem is that there are several different
Which one is actually used seems to be random. Could you clean this up so there is only one import java.net.NetworkInterface
import scala.collection.JavaConverters._
val localIpAddress: String = NetworkInterface.getNetworkInterfaces
.asScala.toSeq.filter(!_.isLoopback)
.flatMap(_.getInetAddresses.asScala.toSeq.filter(_.getAddress.length == 4).map(_.toString))
.headOption.getOrElse(throw TestConfigurationException(s"No suitable network interface found")) |
Strange. There should be only one of course. I will try to find out what is happening. |
Yes, it is three times in the codebase, but these hierarchies are completely separate. A test should only ever see one of those implementations. If it doesn't, then it is a configuration error in some Bazel build file. |
I think that having three classes with the same class name, in the same package, in different parts of the codebase, is asking for trouble with the class loader, which is what has happened here (as I can see because, if I change all three, it works). And it creates a maintenance problem, because if the three classes are redundant and are supposed to be identical, it is too easy to change one and forget to change the others, which also just happened here. Each source file should exist only in one place. |
And I would like to emphasise that I have now wasted several days debugging this problem, which would have been avoided if good basic software development practices had been followed, i.e. no redundant code. |
I agree. We have the same problem with other test classes, too. Most base classes under it and test need to be updated simultaneously in both places. One way would be to merge the test and it hierarchies. Salsah is completely separate, so I'm not really sure, why the classloader sees all three implementations. How did you find it out? |
The problems you are facing and I am trying to help you with is probably local to your system, as the test pass on my machine locally and on Github-CI. As for the redundant code, with SBT it wasn't possible to do it otherwise. If you want to clean up, please go ahead and merge the different test code hierarchies and eliminate the redundant code. |
The problem is in the build file, where the test is defined. There, both the it and test libraries are added as dependencies. This was done in the PR #1724. Then of course, both implementations will be on the classpath. |
That's what I was thinking, too.
This problem started because the admin API depended on API v1. That dependency shouldn't have been there. Then there was the misconfiguration of the test containers, which apparently caused Sipi to run with the wrong configuration. As a result, tests passed when they should have failed. As for the problem on my machine: after looking at https://stackoverflow.com/a/16444863, I think it's just luck that it worked on your system and on GitHub-CI. It seems that you can't assume that
The different test code hierarchies could have been merged under SBT, too. So it was possible not to copy and paste redundant code with SBT.
This is an example of how difficult it is to manage dependencies with Bazel. I think this is a good reason to get rid of Bazel and go back to SBT. |
resolves DSP-1114