Add initial leak test for Android #6691
Conversation
There was a problem hiding this comment.
Reviewed 4 of 11 files at r1.
Reviewable status: 3 of 11 files reviewed, 25 unresolved discussions (waiting on @niklasberglund)
android/test/common/src/main/kotlin/net/mullvad/mullvadvpn/test/common/interactor/AppInteractor.kt line 90 at r1 (raw file):
} fun extractOutIpAddress(): String {
Out is implicit from extract
android/test/common/src/main/kotlin/net/mullvad/mullvadvpn/test/common/interactor/AppInteractor.kt line 100 at r1 (raw file):
.text val extractedIpAddress = outString.split(" ")[1].split(" ")[0]
Why do we have two split twice here? I believe the second split is unnecessary. Also you already have a extractOutIpAddress() that can be reused.
android/test/common/src/main/kotlin/net/mullvad/mullvadvpn/test/common/interactor/AppInteractor.kt line 144 at r1 (raw file):
private fun String.extractOutIpAddress(): String { return split(" ")[1].split(" ")[0]
Same here, .split(" ")[0] should be removable.
android/test/common/src/main/kotlin/net/mullvad/mullvadvpn/test/common/interactor/AppInteractor.kt line 147 at r1 (raw file):
} private fun String.separateInIpAddress(): String {
👍 Good that this is private, since String is a commonly used type so we don't want to pollute the scope of String.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 33 at r1 (raw file):
val trafficGenerator = TrafficGenerator(targetIpAddress, 80) val session = packetCapture.startCapture()
Maybe the api of a capture can be more intuitive. Right now the user must startCapture and stopCapture, another alternative would be something like a scope. Without going into to much code could probably look like something like this:
packetCapture.createSession {
// Create session starts with running startCapture()
// Do stuff where we want to capture the packages
// Once this lambda is complete stop the capture session and return the result.
}
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 35 at r1 (raw file):
val session = packetCapture.startCapture() packetCaptureClient.sendStartCaptureRequest(session) trafficGenerator.startGeneratingUDPTraffic(100)
Reminder to use Duration
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 49 at r1 (raw file):
device.findObjectWithTimeout(By.text("Disconnect")).click() Thread.sleep(2000)
Feels a bit blunt to do Thread.sleep, we should be able to use delay() instead
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 53 at r1 (raw file):
trafficGenerator.stopGeneratingUDPTraffic() packetCaptureClient.sendStopCaptureRequest(session) val streamCollection = packetCapture.stopCapture(session)
It feels a bit redundant where we have to stop twice, doesn't really feel necessary.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 55 at r1 (raw file):
val streamCollection = packetCapture.stopCapture(session) val connectedThroughRelayStartEndDatePair = streamCollection.getConnectedThroughRelayStartEndDate(relayIpAddress)
This variable and function looks like something taken out of ObjectiveC 😅 Try and create a more consice and readable function and variable name.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/Networking.kt line 10 at r1 (raw file):
companion object { fun getIPAddress(): String { NetworkInterface.getNetworkInterfaces()?.toList()?.map { networkInterface ->
As mentioned before, it could probably be worth using !! if there is something we don't expect to fail, or that is completely fatal. It should also fail the test.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/Networking.kt line 14 at r1 (raw file):
?.toList() ?.find { !it.isLoopbackAddress && it is Inet4Address } ?.let {
A bit over use of let, assign to val
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 22 at r1 (raw file):
import kotlinx.serialization.json.put data class PacketCaptureSession(val identifier: String = UUID.randomUUID().toString())
Not sure of the data structures of things here but this sounds more like a SessionId with its current content. Also if it is a UUID, let it be a UUID. Also if it is just an identifier we should consider using a value class instead
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 33 at r1 (raw file):
} suspend fun stopCapture(session: PacketCaptureSession): StreamCollection {
Do we care if this fail? Can we really always guarantee that we will return a StreamCollection?
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 43 at r1 (raw file):
class PacketCaptureClient { private val baseUrl = "http://8.8.8.8"
Hard-coded string, if we want to store this it should probably be a const val and if it is supposed to be with this class it should be in a companion object otherwise it would create one string for each instance of the class.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 44 at r1 (raw file):
class PacketCaptureClient { private val baseUrl = "http://8.8.8.8" private val client =
Preferable we want to inject as much dependencies through the constructor as possible. Not sure of the requirements in this case, e.g isLenient and prettyPrint but we should consider providing this client through the constructor.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 57 at r1 (raw file):
suspend fun sendStartCaptureRequest(session: PacketCaptureSession) { val jsonBody = buildJsonObject { put("label", session.identifier) }
We should be able to create a data structure representing this message and just serializing that. E.g
data class StartCaptureRequest(
val label: PacketCaptureSession
)
and then just serialize it as JSON. Then you don't have to have a hard-coded label in the function.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 61 at r1 (raw file):
Logger.v("Sending start capture request with body: $jsonBody.toString()") val response =
Shouldn't the result be returned some how? Or what happens if this request fails?
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 69 at r1 (raw file):
suspend fun sendStopCaptureRequest(session: PacketCaptureSession) { val response = client.post("$baseUrl/stop-capture/${session.identifier}")
Same here responseis never used. A caller is blind to what happens if something goes wrong.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 73 at r1 (raw file):
suspend fun sendGetCapturedPacketsRequest(session: PacketCaptureSession): HttpResponse { val testDeviceIpAddress = Networking.getIPAddress()
Can we rely on getIPAddress()?
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 86 at r1 (raw file):
@SerialName("tcp") TCP("tcp"), @SerialName("udp") UDP("udp"), @SerialName("icmp") ICMP("icmp")
Do we need the value strings? Do they serve a purpose?
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 97 at r1 (raw file):
val packets: List<Packet> ) { @Contextual var startDate: Date = packets.first().date
As mentioned in our talk, as a general rule data class = no var
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/StreamCollection.kt line 6 at r1 (raw file):
import org.junit.jupiter.api.fail class StreamCollection {
Feels like this class is pretty useless. It is basically just wrapper around List.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/StreamCollection.kt line 7 at r1 (raw file):
class StreamCollection { private var streams: List<Stream> = emptyList()
Avoid use of var
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/StreamCollection.kt line 19 at r1 (raw file):
} fun getConnectedThroughRelayStartEndDate(relayIpAddress: String): Pair<Date, Date> {
Pair is not very descriptive, there should probably be a better type for this. Date range or similar.
https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.ranges/range-to.html
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/StreamCollection.kt line 32 at r1 (raw file):
} for (stream in matchingStreams) {
Not sure exactly what the logic below does, but it feels like it could be simplified a fair bit. If possible we should avoid using
var startDate: Date? = null
var endDate: Date? = null
If we can chain streams and fine this out it would be better.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/TrafficGenerator.kt line 27 at r1 (raw file):
val packet = DatagramPacket(data, data.size, address, destinationPort) timer = Timer()
Never used timer, but it looks a bit weird. There is probably a more intuitive way to do this with coroutines. E.g
val job = yourScope.launch {
// setup with creating packages
while(true) {
socket.send(packet)
delay(interval)
}
}
// and then stop
job.cancel()
The entire thing can then be just made to an extension function on a scope to start sending traffic within a scope.
There was a problem hiding this comment.
Reviewed 2 of 11 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @niklasberglund)
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/StreamCollection.kt line 88 at r2 (raw file):
fun dontAllowTrafficFromTestDevice(toHost: String) { streams.forEach { stream ->
Since we get all packages from all streams in a bunch of places, maybe it could be useful to have some kind of allPackages() helper function?
niklasberglund
force-pushed
the
add-mvp-for-negative-leak-test-droid-824
branch
6 times, most recently
from
44aca84
to
9ea2479
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 19 files reviewed, 26 unresolved discussions (waiting on @Pururun and @Rawa)
android/test/common/src/main/kotlin/net/mullvad/mullvadvpn/test/common/interactor/AppInteractor.kt line 100 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Why do we have two split twice here? I believe the second split is unnecessary. Also you already have a
extractOutIpAddress()that can be reused.
Removed
android/test/common/src/main/kotlin/net/mullvad/mullvadvpn/test/common/interactor/AppInteractor.kt line 144 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Same here,
.split(" ")[0]should be removable.
Removed
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 33 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Maybe the api of a capture can be more intuitive. Right now the user must
startCaptureandstopCapture, another alternative would be something like a scope. Without going into to much code could probably look like something like this:packetCapture.createSession { // Create session starts with running startCapture() // Do stuff where we want to capture the packages // Once this lambda is complete stop the capture session and return the result. }
Packet capture session and traffic generation session are now scope based
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 35 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Reminder to use Duration
Switched to Duration
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 49 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Feels a bit blunt to do Thread.sleep, we should be able to use
delay()instead
Changed to using delay
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 53 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
It feels a bit redundant where we have to stop twice, doesn't really feel necessary.
The capture code was a bit messy before, have been tidied up and using a scoped function instead now
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 55 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
This variable and function looks like something taken out of ObjectiveC 😅 Try and create a more consice and readable function and variable name.
This function is no longer needed so it has been removed, but I have probably introduced some other long names 😅
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 22 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Not sure of the data structures of things here but this sounds more like a SessionId with its current content. Also if it is a UUID, let it be a UUID. Also if it is just an identifier we should consider using a
value classinstead
It is now a UUID
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 33 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Do we care if this fail? Can we really always guarantee that we will return a
StreamCollection?
Have added error handling - failing test if there's an error response
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 57 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
We should be able to create a data structure representing this message and just serializing that. E.g
data class StartCaptureRequest( val label: PacketCaptureSession )and then just serialize it as JSON. Then you don't have to have a hard-coded label in the function.
Ah yes. Hmm now PacketCaptureSession has been removed, using UUID directly instead.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 61 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Shouldn't the result be returned some how? Or what happens if this request fails?
Nothing should be returned, but error handling has been added now
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 69 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Same here
responseis never used. A caller is blind to what happens if something goes wrong.
Nothing should be returned, but error handling has been added now
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 86 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Do we need the value strings? Do they serve a purpose?
No, removed
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 97 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
As mentioned in our talk, as a general rule
data class= novar
Changed
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/TrafficGenerator.kt line 27 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Never used timer, but it looks a bit weird. There is probably a more intuitive way to do this with coroutines. E.g
val job = yourScope.launch { // setup with creating packages while(true) { socket.send(packet) delay(interval) } } // and then stop job.cancel()The entire thing can then be just made to an extension function on a scope to start sending traffic within a scope.
Nice, turned this into a job 👍
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/StreamCollection.kt line 6 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Feels like this class is pretty useless. It is basically just wrapper around List.
This class has been removed
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/StreamCollection.kt line 7 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Avoid use of
var
This class has been removed
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/StreamCollection.kt line 19 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Pair is not very descriptive, there should probably be a better type for this. Date range or similar.
https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.ranges/range-to.html
This class has been removed
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/StreamCollection.kt line 32 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Not sure exactly what the logic below does, but it feels like it could be simplified a fair bit. If possible we should avoid using
var startDate: Date? = null var endDate: Date? = nullIf we can chain streams and fine this out it would be better.
This function is no longer used and has been removed
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/StreamCollection.kt line 88 at r2 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Since we get all packages from all streams in a bunch of places, maybe it could be useful to have some kind of
allPackages()helper function?
This function has been removed
There was a problem hiding this comment.
Reviewed 2 of 13 files at r3, 16 of 16 files at r4, all commit messages.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @niklasberglund and @Rawa)
niklasberglund
force-pushed
the
add-mvp-for-negative-leak-test-droid-824
branch
from
9ea2479
to
8b489e3
Compare
There was a problem hiding this comment.
Reviewable status: 18 of 19 files reviewed, 25 unresolved discussions (waiting on @Pururun and @Rawa)
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/Networking.kt line 10 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
As mentioned before, it could probably be worth using
!!if there is something we don't expect to fail, or that is completely fatal. It should also fail the test.
Turned out these weren't nullable 😯
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/Networking.kt line 14 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
A bit over use of let, assign to
val
Changed
There was a problem hiding this comment.
Reviewable status: 18 of 19 files reviewed, 27 unresolved discussions (waiting on @niklasberglund and @Pururun)
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 49 at r1 (raw file):
Previously, niklasberglund (Niklas Berglund) wrote…
Changed to using delay
There is still Thread.sleep here
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 78 at r5 (raw file):
lateinit var captureResult: PacketCaptureResult val connectTime = DateTime.now() TrafficGenerator(targetIpAddress, targetPort).generateTraffic(10.milliseconds) {
Shouldn't this be the other way around? Now we would start generating traffic and then start capturing, thus we might miss some packets until the capture has started. Also then we don't have to do:
lateinit var captureResult: PacketCaptureResult
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 93 at r5 (raw file):
val capturedStreams = captureResult.streams val capturedPcap = captureResult.pcap
This is not used by anything right now, should it be removed?
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 95 at r5 (raw file):
val capturedPcap = captureResult.pcap // Attachment.saveAttachment("capture.pcap", "should-leak", capturedPcap)
Should this still be here?
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 152 at r5 (raw file):
// val pcapContent = packetCapture.getPcap() // Attachment.saveAttachment("capture.pcap", "should-leak", pcapContent.toByteArray())
Remove?
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/Networking.kt line 10 at r1 (raw file):
Previously, niklasberglund (Niklas Berglund) wrote…
Turned out these weren't nullable 😯
They can be null, Enumeration<NetworkInterface>!. The ! notifies you can Kotlin can determine nullability since it invokes java code.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/Networking.kt line 7 at r5 (raw file):
import org.junit.Assert.fail class Networking {
Do we have a usage for this class? Otherwise getIpAddress can just be a function straight away
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 22 at r1 (raw file):
Previously, niklasberglund (Niklas Berglund) wrote…
It is now a
UUID
Not sure if you missed to push latest, but It is still a String here.
Edit: Saw that you said further down that you've removed this class, I think it shouldn't be removed, we should use a value class containing a UUID, it is more clean and less error prone.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 55 at r5 (raw file):
} suspend fun capturePackets(block: suspend () -> Unit): PacketCaptureResult = runBlocking {
Is it intented to use runBlocking here? To wait in the test case?
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 60 at r5 (raw file):
stopCapture() val parsedCapture = getParsedCapture() val pcapString = getPcap()
Maybe there should be a private finishCapture(): PackertCaptureResult that does all these 3 things?
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 137 at r5 (raw file):
data class Host(val ipAddress: String, val port: Int) object PacketSerializer : KSerializer<List<Packet>> {
Is this really needed? Feels like you should be able to let serializationx do this for you? Or was it the boolean thing causing this to be the case?
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 182 at r5 (raw file):
destinationAddressAndPort.split(":").first(), destinationAddressAndPort.split(":").last().toInt(), )
We should create a function on Host.Companion that allows you to create the type from a String. So you can do like this:
Host.fromString(destinationAddressAndPort)
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 191 at r5 (raw file):
@Contextual val rxEndDate: DateTime? = packets.lastOrNull { !it.fromPeer }?.date fun getTxPackets(): List<TxPacket> = packets.filterIsInstance<TxPacket>()
generally one would avoid "get" it is very java like.
fun txPackets()
fun rxPackets()
or if it sees a lot of reuse:
val txPackets by lazy { packets.filterIsInstance<TxPacket>() }
val rxPackets by lazy { packets.filterIsInstance<RxPacket>() }
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 197 at r5 (raw file):
@Serializable sealed class Packet {
sealed interface instead of sealed class
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/TrafficGenerator.kt line 32 at r5 (raw file):
"TrafficGenerator sending UDP packet to $destinationHost:$destinationPort" ) delay(interval.toLong(DurationUnit.MILLISECONDS))
I believe you can use delay with a Duration?
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 27 unresolved discussions (waiting on @niklasberglund)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @niklasberglund)
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 57 at r1 (raw file):
Previously, niklasberglund (Niklas Berglund) wrote…
Ah yes. Hmm now
PacketCaptureSessionhas been removed, usingUUIDdirectly instead.
Still, present, we should avoid using buildJsonObject and instead use serialize from kotlinx.serialization.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 32 at r5 (raw file):
class PacketCapture { private val client = PacketCaptureClient() private var sessionUUID = UUID.randomUUID()
Should not var right?
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 96 at r5 (raw file):
val jsonBody = buildJsonObject { put("label", sessionUUID.toString()) } Logger.v("Sending start capture request with body: $jsonBody.toString()")
You are not invoking toString()
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 106 at r5 (raw file):
suspend fun sendStopCaptureRequest(sessionUUID: UUID) { val response = httpClient.post("$baseUrl/stop-capture/${sessionUUID.toString()}")
Remove response if we don't use it.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @niklasberglund)
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 53 at r5 (raw file):
Logger.v("PCAP content: ${pcapContent.body<String>()}") return pcapContent.body<String>() }
Consider
private suspend fun getPcap() = client.sendGetPcapFileRequest(sessionUUID).pcapContent.body<String>().also {
Logger.v("PCAP content: $it")
}
private suspend fun getPcap(): String {
return client.sendGetPcapFileRequest(sessionUUID)
.pcapContent.body<String>()
.also {
Logger.v("PCAP content: $it")
}
}
d5b61cc
to
dd33077
Compare
There was a problem hiding this comment.
Reviewable status: 15 of 19 files reviewed, 26 unresolved discussions (waiting on @Pururun and @Rawa)
android/test/common/src/main/kotlin/net/mullvad/mullvadvpn/test/common/interactor/AppInteractor.kt line 90 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Out is implicit from
extract
"Out" refers to the out IP address
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 49 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
There is still
Thread.sleephere
Replaced with delay
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 78 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
Shouldn't this be the other way around? Now we would start generating traffic and then start capturing, thus we might miss some packets until the capture has started. Also then we don't have to do:
lateinit var captureResult: PacketCaptureResult
Doing it the other way around now
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 93 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
This is not used by anything right now, should it be removed?
See answer below https://reviewable.io/reviews/mullvad/mullvadvpn-app/6691#-O6RGIMC6kToCq8sLzIi
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/LeakTest.kt line 95 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
Should this still be here?
Indecisive. There's a separate Linear issue for storing PCAP files, but most of that ticket has actually been implemented in this PR already. https://linear.app/mullvad/issue/DROID-1315/store-pcap-files-from-packet-captures-on-device
I'm leaning towards implementing this issue as well in this PR. How do you think?
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/Networking.kt line 7 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
Do we have a usage for this class? Otherwise
getIpAddresscan just be a function straight away
Good point, but I'm anticipating that this class will grow. For iOS end to end tests there's a "Networking" class which is quite big https://github.com/mullvad/mullvadvpn-app/blob/dc5544701738ad3fab4738bc9314ef95e8d6731e/ios/MullvadVPNUITests/Networking/Networking.swift
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 22 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Not sure if you missed to push latest, but It is still a
Stringhere.Edit: Saw that you said further down that you've removed this class, I think it shouldn't be removed, we should use a
value classcontaining a UUID, it is more clean and less error prone.
Turned this into a UUID and passing around PacketCatpureSession data class objects instead of UUID
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 43 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Hard-coded string, if we want to store this it should probably be a
const valand if it is supposed to be with this class it should be in acompanion objectotherwise it would create one string for each instance of the class.
Moved to companion object
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 44 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Preferable we want to inject as much dependencies through the constructor as possible. Not sure of the requirements in this case,
e.g isLenientandprettyPrintbut we should consider providing this client through the constructor.
It looks like they cannot be set through the constructor in this case unless I'm missing something, but it's good to know. I'll keep this in mind.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 57 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Still, present, we should avoid using
buildJsonObjectand instead use serialize from kotlinx.serialization.
Replaced with a serializable data class
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 73 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Can we rely on
getIPAddress()?
I think so? 😊
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 32 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
Should not
varright?
Yes, changed to val
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 96 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
You are not invoking
toString()
Oops 😄 Now it's being invoked.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 106 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
Remove response if we don't use it.
Removed
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 137 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
Is this really needed? Feels like you should be able to let serializationx do this for you? Or was it the boolean thing causing this to be the case?
Myea, I'm not 100% sure but from what I have read the polymorphism offered out of the box is to separate between classes with @SerialName("expected-type-value-here") which maps based on the value of a type attribute. We could request a type attribute to be added, but maybe that would be something that can be improved later?
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 182 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
We should create a function on
Host.Companionthat allows you to create the type from a String. So you can do like this:
Host.fromString(destinationAddressAndPort)
Yes that's much better. Using this approach now. Is fromString the common convention for naming a factory method or should it have some other name?
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 191 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
generally one would avoid "get" it is very java like.
fun txPackets() fun rxPackets()or if it sees a lot of reuse:
val txPackets by lazy { packets.filterIsInstance<TxPacket>() } val rxPackets by lazy { packets.filterIsInstance<RxPacket>() }
Renamed
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 197 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
sealed interfaceinstead ofsealed class
Changed
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/TrafficGenerator.kt line 32 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
I believe you can use
delaywith aDuration?
True, changed 👍
niklasberglund
force-pushed
the
add-mvp-for-negative-leak-test-droid-824
branch
2 times, most recently
from
9f2c1d3
to
f5d96a5
Compare
There was a problem hiding this comment.
Reviewable status: 13 of 19 files reviewed, 26 unresolved discussions (waiting on @Pururun and @Rawa)
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/Networking.kt line 10 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
They can be null,
Enumeration<NetworkInterface>!. The!notifies you can Kotlin can determine nullability since it invokes java code.
Not-null asserting getNetworkInterfaces()
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 53 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
Consider
private suspend fun getPcap() = client.sendGetPcapFileRequest(sessionUUID).pcapContent.body<String>().also { Logger.v("PCAP content: $it") }private suspend fun getPcap(): String { return client.sendGetPcapFileRequest(sessionUUID) .pcapContent.body<String>() .also { Logger.v("PCAP content: $it") } }
Nice suggestion 👍 Actually the the PCAP content was just debug logging which should've been removed, so I removed it now.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 55 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
Is it intented to use
runBlockinghere? To wait in the test case?
It is intended, I believe it is required because suspend functions are invoked? Or is there some other better way?
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 60 at r5 (raw file):
Previously, Rawa (David Göransson) wrote…
Maybe there should be a
private finishCapture(): PackertCaptureResultthat does all these 3 things?
This is already in the relatively low level class PacketCapture so I think adding an abstraction layer might make it difficult to follow. The capturePackets block function is already an abstraction.
There was a problem hiding this comment.
Reviewed 6 of 16 files at r18, 2 of 3 files at r21.
Reviewable status: 29 of 34 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad, @niklasberglund, and @Pururun)
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/model/Packet.kt line 18 at r20 (raw file):
data class RxPacket( @SerialName("timestamp") @Contextual override val date: DateTime, @SerialName("from_peer") override val fromPeer: Boolean,
Should be hard-coded true/false for RxPacket/TxPacket.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/serializer/PacketSerializer.kt line 14 at r20 (raw file):
object PacketSerializer : JsonContentPolymorphicSerializer<Packet>(Packet::class) { override fun selectDeserializer(element: JsonElement): KSerializer<out Packet> {
🔥
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/util/SplitStreamList.kt line 6 at r20 (raw file):
import org.joda.time.Interval fun splitStreamList(streamList: List<Stream>, interval: Interval): List<Stream> {
Should be extension function.
fun List<Stream>.split() = filter { interval.contains(it.interval) }
There was a problem hiding this comment.
Reviewed 2 of 16 files at r18, 1 of 1 files at r20, all commit messages.
Reviewable status: 32 of 34 files reviewed, 3 unresolved discussions (waiting on @niklasberglund and @Pururun)
There was a problem hiding this comment.
Reviewable status: 32 of 34 files reviewed, 3 unresolved discussions (waiting on @niklasberglund and @Pururun)
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/util/SplitStreamList.kt line 6 at r20 (raw file):
Previously, Rawa (David Göransson) wrote…
Should be extension function.
fun List<Stream>.split() = filter { interval.contains(it.interval) }
Also, do you actually split the stream? Looks like we only remove stream outside.
There was a problem hiding this comment.
Reviewable status: 32 of 34 files reviewed, 4 unresolved discussions (waiting on @niklasberglund and @Pururun)
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/LeakCheck.kt line 9 at r21 (raw file):
object LeakCheck { fun assertNoLeaks(streams: List<Stream>, rules: List<LeakRule>) { // Assert that there are streams to be analyzed
Maybe add a clarification that a Stream that has no packets can't be created.
There was a problem hiding this comment.
Reviewed 1 of 16 files at r18.
Reviewable status: 33 of 34 files reviewed, 7 unresolved discussions (waiting on @niklasberglund and @Pururun)
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 49 at r21 (raw file):
private suspend fun getParsedCapture(): List<Stream> { val parsedPacketsResponse = client.sendGetCapturedPacketsRequest(session) val capturedStreams = parsedPacketsResponse.body<List<Stream>>()
To avoid unnecessary assignment:
return parsedPacketsResponse.body<List<Stream>>().also {
Logger.v("Captured streams: $it")
}
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 64 at r21 (raw file):
val parsedCapture = getParsedCapture() val pcap = getPcap() return PacketCaptureResult(parsedCapture, pcap)
Join last 3 rows into one statement.
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 106 at r21 (raw file):
Logger.v("Sending start capture request with body: ${Json.encodeToString(jsonObject)}") val response =
Response never used?
There was a problem hiding this comment.
Reviewed 1 of 3 files at r21.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @niklasberglund)
niklasberglund
force-pushed
the
add-mvp-for-negative-leak-test-droid-824
branch
from
729d8ba
to
dd07d68
Compare
There was a problem hiding this comment.
Reviewable status: 29 of 34 files reviewed, 7 unresolved discussions (waiting on @albin-mullvad and @Rawa)
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/LeakCheck.kt line 9 at r21 (raw file):
Previously, Rawa (David Göransson) wrote…
Maybe add a clarification that a Stream that has no packets can't be created.
Added clarification
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 43 at r14 (raw file):
Previously, Rawa (David Göransson) wrote…
Ping, still here
Removed for real
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 49 at r21 (raw file):
Previously, Rawa (David Göransson) wrote…
To avoid unnecessary assignment:
return parsedPacketsResponse.body<List<Stream>>().also { Logger.v("Captured streams: $it") }
Done
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 64 at r21 (raw file):
Previously, Rawa (David Göransson) wrote…
Join last 3 rows into one statement.
Joined
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/PacketCapture.kt line 106 at r21 (raw file):
Previously, Rawa (David Göransson) wrote…
Response never used?
Not assigning to variable any more
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/model/Packet.kt line 18 at r20 (raw file):
Previously, Rawa (David Göransson) wrote…
Should be hard-coded true/false for RxPacket/TxPacket.
Hard coded true/false
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/util/SplitStreamList.kt line 6 at r20 (raw file):
Previously, Rawa (David Göransson) wrote…
Also, do you actually split the stream? Looks like we only remove stream outside.
Removed this function and file because its no longer used
niklasberglund
force-pushed
the
add-mvp-for-negative-leak-test-droid-824
branch
from
dd07d68
to
29382e8
Compare
|
Fresh of the boat: https://github.com/ktorio/ktor/releases/tag/3.0.0 |
There was a problem hiding this comment.
Reviewed 3 of 5 files at r22, 1 of 1 files at r23, all commit messages.
Reviewable status: 33 of 34 files reviewed, 2 unresolved discussions (waiting on @niklasberglund)
android/test/e2e/e2e.properties line 4 at r23 (raw file):
ENABLE_HIGHLY_RATE_LIMITED_TESTS=false ENABLE_ACCESS_TO_LOCAL_API_TESTS=false TRAFFIC_GENERATION_IP_ADDRESS=45.83.223.209
Should these values really be checked in?
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/model/Packet.kt line 18 at r20 (raw file):
Previously, niklasberglund (Niklas Berglund) wrote…
Hard coded true/false
You set the default constructor value to be true/false, this allows for it to be changed. It should be like this:
data class RxPacket(
@SerialName("timestamp") @Contextual override val date: DateTime,
) : Packet
@SerialName("from_peer") override val fromPeer = false,
Like this a creator of RxPacket cannot change the fromPeer value.
There was a problem hiding this comment.
Reviewed 1 of 5 files at r22.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @niklasberglund)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @niklasberglund)
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/model/Packet.kt line 18 at r20 (raw file):
Previously, Rawa (David Göransson) wrote…
You set the default constructor value to be true/false, this allows for it to be changed. It should be like this:
data class RxPacket( @SerialName("timestamp") @Contextual override val date: DateTime, ) : Packet @SerialName("from_peer") override val fromPeer = false,Like this a creator of RxPacket cannot change the
fromPeervalue.
Edit, missed the brackets:
data class RxPacket(
@SerialName("timestamp") @Contextual override val date: DateTime,
) : Packet {
@SerialName("from_peer") override val fromPeer = false
}
c6027e9
to
bbbf3cc
Compare
There was a problem hiding this comment.
Updated to ktor 3.0.0
Reviewable status: 30 of 34 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @Rawa)
niklasberglund
force-pushed
the
add-mvp-for-negative-leak-test-droid-824
branch
from
bbbf3cc
to
8877841
Compare
There was a problem hiding this comment.
Reviewed 4 of 4 files at r24, 1 of 1 files at r25, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @niklasberglund)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)
android/test/e2e/e2e.properties line 4 at r23 (raw file):
Previously, Rawa (David Göransson) wrote…
Should these values really be checked in?
It's a good question. I think the answer is yes, but there is no optimal solution for overriding these locally on a developers computer today. It should be as easy as specifying your preferred value i local.properties but today doing so won't override these values. So today the way to change is either by running tests from CLI and specify these properties as arguments, or by locally editing e2e.properties. Created an issue for improving this https://linear.app/mullvad/issue/DROID-1430/localproperties-values-should-override-e2eproperties-values
android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/model/Packet.kt line 18 at r20 (raw file):
Previously, Rawa (David Göransson) wrote…
Edit, missed the brackets:
data class RxPacket( @SerialName("timestamp") @Contextual override val date: DateTime, ) : Packet { @SerialName("from_peer") override val fromPeer = false }
Ahhh yes. Updated
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
android/test/e2e/e2e.properties line 4 at r23 (raw file):
Previously, niklasberglund (Niklas Berglund) wrote…
It's a good question. I think the answer is yes, but there is no optimal solution for overriding these locally on a developers computer today. It should be as easy as specifying your preferred value i
local.propertiesbut today doing so won't override these values. So today the way to change is either by running tests from CLI and specify these properties as arguments, or by locally editinge2e.properties. Created an issue for improving this https://linear.app/mullvad/issue/DROID-1430/localproperties-values-should-override-e2eproperties-values
Discussed offline, we'll fix this in the other issue
There was a problem hiding this comment.
Reviewed 1 of 13 files at r3, 1 of 16 files at r4, 1 of 3 files at r15, 7 of 12 files at r16, 3 of 16 files at r18.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
Rawa
force-pushed
the
add-mvp-for-negative-leak-test-droid-824
branch
from
8877841
to
d8ffbd6
Compare
Rawa
force-pushed
the
add-mvp-for-negative-leak-test-droid-824
branch
from
d8ffbd6
to
d6e69a7
Compare
MVP of initial leak test - traffic is sent to a host and the tests check if traffic leak outside the tunnel.
Note that there's an issue with timestamps from the packets in the parsed packet capture now, this must be resolved before this PR is merged. So the PR might still be updated.
This change is