Android: Extract parsing command line file to a separate class + add unit tests

[melquiadess]

This PR

• extracts _cl_ file (containing command line params) parsing to a separate class
• adds parameterized unit tests for it
• resets the buffer to 0s in parseCommandLine() before reading the next header
• refactors the return type (and relevant code) of parseCommandLine() from mutable lists to an immutable one (immutable collections are preferred)

Rationale behind unit tests structure

The usual structure for unit tests would be:

lib
  |____src
         |____main
         |       |____com.google.android.vending
         |       |____com.godotengine.godot
         |            
         |____test

where main is marked as sources root, and test as test sources root. In this case though, the src already contains 2 packages, and adding test package and marking it as test sources root didn't seem to be ok for gradle (I'm guessing because of the nesting of test sources root inside sources root). An option would be to add main, mark it as sources root, and move the 2 existing packages (com.google.android.vending and com.godotengine.godot) there, but the resulting diff would be considerably bigger, and perhaps with some consequences I don't yet predict, not knowing the codebase well...).

Instead, I've opted for the following:

lib
  |____src
  |      |____com.google.android.vending
  |      |____com.godotengine.godot
  |
  |____srcTest          

with simple addition in sourceSets as test.java.srcDirs = ['srcTest/java']. Unless I'm missing something (please comment), this works and results in the smallest diff.

Unit testing parseCommandLine()

I have experimented with creating and including various _cl_ files containing different command line args, but eventually went with the byte array constant representations of those files, and tinkered with their variations to tests the function fully, but please let me know if I've missed some scenarios/edge cases.

[melquiadess]

This is new, and I'm not sure about it, but I have a feeling that we should reset the buffer before reading the next header, in case the new header leads to a shorter string, and so we don't want the old one still partially occupying the array... 🤔

[m4gr3d]

That should be taken care of by checking that argBytes < 4; Inputstream#read(...) returns the number of bytes it read, and in this scenario we expect 4 bytes describing how long the next String will be and thus fully filling up the headerBytes byte array.

Are you able to validate the scenario you described using the unit tests?

[melquiadess]

I see, perhaps I misunderstood, thanks 👍
Not verified with unit test, as I wasn't sure what would the real world scenario be...

Let me think on it a bit again. If anything, I can remove if this is covered by the argBytes < 4 check

[melquiadess]

Are you able to validate the scenario you described using the unit tests?

No, can't think of an edge case like that atm, which wouldn't be cought by argBytes < 4 check

Removed the extra line, thanks

[m4gr3d]

That should be taken care of by checking that argBytes < 4; Inputstream#read(...) returns the number of bytes it read, and in this scenario we expect 4 bytes describing how long the next String will be and thus fully filling up the headerBytes byte array.

Are you able to validate the scenario you described using the unit tests?

[m4gr3d]

Why listOf() instead of mutableListOf()?

[melquiadess]

Generally immutable collections are preferred over mutable ones where possible.

[m4gr3d]

In general I'd agree, however I can see us departing from that approach for a couple of reasons:

• For public Godot apis (as the CommandLineFileParser is currently), we've taken the approach of providing flexibility to developers integrating those apis especially as the majority is likely to have little kotlin / java expertise. For example, see Android: Fix UnsupportedOperationException remove from non-ArrayList #89703 (comment) for reference.
• If CommandLineFileParser is intended to be a non-public api (in which case we'll need to mark it as internal) then it only has one call-site which immediately adds the returned elements into a MutableList, so we may as well have the return value be a MutableList, removing the need for that extra step.

[melquiadess]

Oh, great, thanks for the context!

On the first point, I guess the preferred way would be to favour consistency indeed in that case, and so to return a mutable list 👍 (I personally would prefer the non-mutable list for its benefits and as a good practice, but there's always a possibility to open a proposal and discuss, so all good on my side)

On the second point - I'd argue that the extra step would be acceptable with the (nicer) following:

private fun getCommandLine(): MutableList<String> {
	val commandLine = commandLineFileParser
		.parseCommandLine(requireActivity().assets.open("_cl_"))
		.toMutableList()

	val hostCommandLine = primaryHost?.commandLine
	if (!hostCommandLine.isNullOrEmpty()) {
		commandLine.addAll(hostCommandLine)
	}

	return commandLine
}

But, I am not sure whether CommandLineFileParser should be public or not... given your experience with Godot Android - what's your take?

---

TLDR;
I'll revert to the mutable list soon

[melquiadess]

Done

[m4gr3d]

Looks great!

But, I am not sure whether CommandLineFileParser should be public or not...

Let's keep the class internal for now. This provides us with the flexibility to update the api if needed, whereas if we make it public, we have to keep the api consistent to maintain backward compatibility.

[melquiadess]

Looks great!

But, I am not sure whether CommandLineFileParser should be public or not...

Let's keep the class internal for now. This provides us with the flexibility to update the api if needed, whereas if we make it public, we have to keep the api consistent to maintain backward compatibility.

Sounds good, done.

[akien-mga]

Thanks!