Skip to content
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

Invalid file names are generated because of test names #263

Closed
TWiStErRob opened this issue Mar 10, 2022 · 11 comments
Closed

Invalid file names are generated because of test names #263

TWiStErRob opened this issue Mar 10, 2022 · 11 comments
Assignees
Labels

Comments

@TWiStErRob
Copy link
Contributor

TWiStErRob commented Mar 10, 2022

I'm having these tests in src/androidTest/kotlin:

class ColorUtilsKtTest {

	class ReplaceAlphaFrom(
		val useCase: String,
		@ColorInt val receiver: Int,
		@ColorInt val param: Int,
		@ColorInt val expected: Int,
	)

	@TestFactory fun replaceAlphaFrom() =
		listOf(
			ReplaceAlphaFrom(
				"Replaces alpha keeping RGB",
				Color.argb(0x12, 0x34, 0x56, 0x78),
				Color.argb(0x87, 0x65, 0x43, 0x21),
				Color.argb(0x87, 0x34, 0x56, 0x78),
			),
			ReplaceAlphaFrom(
				"Adds only alpha",
				Color.argb(0x00, 0x00, 0x00, 0x00),
				Color.argb(0xAB, 0x00, 0x00, 0x00),
				Color.argb(0xAB, 0x00, 0x00, 0x00),
			),
			ReplaceAlphaFrom(
				"Clears alpha",
				Color.argb(0x12, 0x34, 0x56, 0x78),
				Color.argb(0x00, 0x00, 0x00, 0x00),
				Color.argb(0x00, 0x34, 0x56, 0x78),
			),
		).map {
			dynamicTest(it.useCase) {
				val result = it.receiver.replaceAlphaFrom(it.param)

				assertThat(it.expected, equalTo(result))
			}
		}
}

When running them in Android Studio I get
image
which is beautiful, well done!

I have the same structure in src/test/kotlin too, that looks like this:
image

At this point I can see that there's a limitation of nesting test containers in androidTest and you worked around it by concat-ing. I've identified this line to be responsible:


Now, onto the issue...
When running on AGP 7.1 (gradlew :feature:base:connectedCheck) there are some new features to dump the logcat and other files, see what is the output here:
image

This is on Windows, where there are reserved characters in file names (see also wiki. You can see this in action, because build/outputs/androidTest-results/connected/API_29(AVD) - 10/logcat-net.twisterrob.colorfilters.android.ColorUtilsKtTest-replaceAlphaFrom file name is truncated at the :, and actually there's only 1 file, not 3. (AGP issue)

When running the same on Mac (CI) I get the full name: build/outputs/androidTest-results/connected/test(AVD) - 10/logcat-net.twisterrob.colorfilters.android.ColorUtilsKtTest-replaceAlphaFrom: Adds only alpha.txt, but when trying to upload artifacts I get an error because of the "invalid" character: https://github.com/TWiStErRob/net.twisterrob.colorfilters/runs/5487958432?check_suite_focus=true#step:7:17
(AGP issue)

image

At this point I would like to ask to make it configurable what the joiner characters are, e.g. I would configure it to - to prevent this error. Related android-test issue: android/android-test#1314

Repro: https://github.com/TWiStErRob/repros/tree/master/agp/junit5-logcat-filenames

TWiStErRob added a commit to TWiStErRob/net.twisterrob.colorfilters that referenced this issue Mar 10, 2022
TWiStErRob added a commit to TWiStErRob/net.twisterrob.colorfilters that referenced this issue Mar 22, 2022
* Add CI based on net.twisterrob.sun
* Make gradlew executable
* Remove unused files
* Workaround for mannodermaus/android-junit5#263
* Increase timeout
@mannodermaus mannodermaus self-assigned this Mar 27, 2022
@mannodermaus
Copy link
Owner

Thanks for bringing this up and the thorough description! 🙏 Ideally we'd like to retain the colon characters for test reporting, but sanitize the handoff to this Logcat task for generation of its file names. Gotta dive into how this new implementation of connectedCheck pulls these files.

@mannodermaus
Copy link
Owner

mannodermaus commented Apr 9, 2022

I have entered the rabbit hole that is the Unified Test Platform for Android (UTP) and wanted to share some initial findings. UTP was apparently introduced with AGP 7.0 and takes care of the test reporting for instrumentation tests through a variety of plugins (here's a list of all the Maven artifacts for these plugins).

I found UtpTestUtils.kt inside AGP 7.1, where the output folder for the executing device is created (the one that stores test-result.pb). The remaining contents of the folder are handled by various"host plugins", including one for Logcat. Inside this Logcat dependency, the main plugin class is generating the problematic file names directly from the contents of Logcat itself (source link 1, source link 2):

private fun parseLine(line: String) {
    val testPackageClassAndMethodNames = line.split("TestRunner: started: ")[1].split("(")
    val testMethod = testPackageClassAndMethodNames[0].trim()
    val testPackageAndClass = testPackageClassAndMethodNames[1].removeSuffix(")")
    val tempFileName = generateLogcatFileName(testPackageAndClass, testMethod) // <----- !!
    tempLogcatFile = File(tempFileName)
    logcatFilePaths.add(tempFileName)
    tempLogcatWriter = tempLogcatFile.outputStream().bufferedWriter()
}

private fun generateLogcatFileName(
        testPackageAndClass: String,
        testMethod: String
) = File(outputDir, "logcat-$testPackageAndClass-$testMethod.txt").absolutePath // <----- !!

I can't really change the output of Logcat, though - else the IDE reporting and other places would start to look messy again. I wonder if the fix could be applied from AGP's side, cleaning the generated file names of any illegal symbols. We managed to fix other upstream bugs in the past so maybe this could be another attempt at one. 🤔

@TWiStErRob
Copy link
Contributor Author

TWiStErRob commented Apr 9, 2022

Awesome investigation, brave soul embarking on a journey into the rabbit hole. 🙏

I opened a bug for AGP too, would you mind commenting on the issue with your findings?

In the meantime please consider making a change to allow for at least making it work in some way. A configuration param that's off by default, or even just mutable private field that can be set with reflection 🥺. "Ugly working" is better than "pretty, but broken".

@mannodermaus
Copy link
Owner

mannodermaus commented Apr 10, 2022

I copied my findings to the ticket on the Google issue tracker, thanks for the reminder. As for a workaround from android-junit5's side, the easiest thing I can think of is to not use colons in the test output at all. Replacing the : separator with - will change the reporting in the IDE, but should stop breaking the file generation of the Logcat host plugin.

Before After
Screen Shot 2022-04-10 at 19 45 13 Screen Shot 2022-04-10 at 19 47 31

Screen Shot 2022-04-10 at 19 56 50

The remaining punctuation in these file names (comma, square brackets) should not pose a problem for Windows, right?

@TWiStErRob
Copy link
Contributor Author

TWiStErRob commented Apr 10, 2022

Confirmed that Windows has no issues with these characters:
-. ()[]

Screenshot below for logcat-de.mann.sample.Test.f(RepetitionInfo, AcitivtyScenario) - [1] repetition 1 of 3-repeatedTestExample.txt in Windows Explorer (Win 10):
image

@TWiStErRob
Copy link
Contributor Author

Note: there's something weird in your screenshots, the open ( is missing.

@TWiStErRob
Copy link
Contributor Author

TWiStErRob commented Apr 10, 2022

Btw, I tried to type a : and Windows told me this:
image

Mind you, this means that &, ;, {/}, %, =, ', !, #, @, $ are all valid characters.

image

@mannodermaus
Copy link
Owner

Thanks for confirming!

Note: there's something weird in your screenshots, the open ( is missing.

Yeah, I noticed that too. It's likely a side effect of this Logcat plugin splitting the test name by ( and just using the first two elements of the result, which doesn't bode well when the test name itself already has parentheses.

In any case, version 1.3.1 of the instrumentation test libraries should have this fix. If you need it now, please use their -SNAPSHOT instead!

@TWiStErRob

This comment was marked as resolved.

@mannodermaus
Copy link
Owner

mannodermaus commented Apr 10, 2022

Ah no, sorry - I was referring to the fix on android-junit5's side:

dependencies {
  androidTestImplementation("de.mannodermaus.junit5:android-test-core:1.3.1-SNAPSHOT")
  androidTestRuntimeOnly("de.mannodermaus.junit5:android-test-runner:1.3.1-SNAPSHOT")
}

Also, noted about the edge case with the parentheses. Square brackets could work as an alternative for our own test naming, I suppose, though I'm hoping for an AGP-side resolution first before considering that one, to be honest.

@mannodermaus mannodermaus added this to the Instrumentation 1.4.0 milestone Sep 17, 2023
@mannodermaus
Copy link
Owner

Released in 1.4.0 of the instrumentation libraries

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants