-
Notifications
You must be signed in to change notification settings - Fork 44
#125 Report Path sanitization #126
base: master
Are you sure you want to change the base?
Conversation
|
quick fix may need cleanup |
|
@artem-zinnatullin have a look when u get a chance |
| } | ||
| } | ||
|
|
||
| fun AdbDevice.sanitizedId() = id.replace(":","-") |
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.
IMO it would be great to create simple data class for id with an extension sanitized() to avoid duplication of those methods here and in Main.kt. Or even having separate val sanitized for storing sanitized path. WDYT?
artem-zinnatullin
left a comment
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.
Sorry, somehow it went below my radar, just noticed it
| writeJunit4Report( | ||
| suite = adbDeviceTestRun.toSuite(args.testPackage), | ||
| outputFile = File(File(args.outputDirectory, "junit4-reports"), "${device.id}.xml") | ||
| outputFile = File(File(args.outputDirectory, "junit4-reports"), "${device.sanitizedId()}.xml") |
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.
hm, this might break someones report parsing, we should emphasize this in release notes
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.
There is also an edge case when filenames for multiple devices will be equal after sanitization e.g. both 123:456 and 123-456.
| val instrumentationOutput: File | ||
| ) | ||
| ){ | ||
| fun sanitizedId() = id.replace(":","-") |
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.
Probably a property would look better, but I don't have strong opinion :)
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.
We need tests btw :)
| } | ||
| } | ||
|
|
||
| fun AdbDevice.sanitizedId() = id.replace(":","-") |
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.
That's not good, we shouldn't duplicate implementation, it'll be easy to de-sync them
|
I will work on the feedback once I find some time, just busy with building new infra so might not need this anymore cuz my Jenkins master is now a Linux machine :) |
#125
fix the report paths when
:is part of device id connected to adb replace with-