search

MiniDNS / minidns

DNS library for Android and Java SE
Other
215 stars 61 forks source link

ClassNotFoundException when running in UnitTest #120

Closed rekire closed 2 years ago

rekire commented 2 years ago

I'm evaluating this library for a project and I started using it in a UnitTest, but it crashes:

WARNUNG: Exception in findDNSByReflection
java.lang.ClassNotFoundException: android.os.SystemProperties
    at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:581)
    at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
    at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
    at java.base/java.lang.Class.forName0(Native Method)
    at java.base/java.lang.Class.forName(Class.java:315)
    at org.minidns.dnsserverlookup.AndroidUsingReflection.getDnsServerAddresses(AndroidUsingReflection.java:37)
    at org.minidns.DnsClient.findDNS(DnsClient.java:253)
    at org.minidns.DnsClient.findDnsAddresses(DnsClient.java:309)
    at org.minidns.DnsClient.getServerAddresses(DnsClient.java:102)
    at org.minidns.DnsClient.query(DnsClient.java:147)
        ...

In my opinion you should check first if you are running on Android before executing Android specific code. I just want to quote the first sentence of the readme: "MiniDNS [...] is a DNS library for Android and Java SE." A unit test falls in my opinion in that case.

I like the idea of fallback to a fixed DNS server, but why just one IPv4 and IPv6? Google has 2 of both and there are also public alternatives like 1.1.1.1 or 9.9.9.9

When I find some free time I will create a PR in order to fix that. If anyone else is quicker then go on, that part is no rocket science.

Flowdalic commented 2 years ago

In my opinion you should check first if you are running on Android before executing Android specific code.

You mean like https://github.com/MiniDNS/minidns/blob/e0a137f1dcc6fbcbce0d0d35c502bc3ec9f2c765/minidns-client/src/main/java/org/minidns/dnsserverlookup/AndroidUsingReflection.java#L76-L78

I am not sure why this doesn't work for you. How do you invoke the unit tests?

I like the idea of fallback to a fixed DNS server, but why just one IPv4 and IPv6?

Hardcoded DNS servers are a sensitive topic due the privacy implications. I tend to change MiniDNS so that they are not populated by default, and provide convenience methods like addGoogleDnsServersAsFallback(). Furthermore there is currently no method to modify the hardcoded server set (you can do it via reflection though). Please also check the existing issues around hardcoded servers, like #72 and #102.

rekire commented 2 years ago

Thank you for your answer. After reviewing my code I found out that it is not crashing, but it is logging that exception. However since I still get the error logged that PlatformDetection seems to be not bulletproof. Might be a check for Class.forName("org.junit.Test") could fix it.

To be honest I just executed the test from Android Studio, I would guess that this executes in the background e.g. gradle test task.

Flowdalic commented 2 years ago

I think what basically happens is that Android Studio executes the unit tests in an Android(-ish) environment, so that PlatformDetection.isAndroid()returnstrue. However modern Android versions to no longer provide access toandroid.os.SystemProperties`. Hence the exception.

Might be a check for Class.forName("org.junit.Test") could fix it.

How would that fix it?

rekire commented 2 years ago

That code could detect if junit is present and therefor not on a real device, however your solution is much better.