Skip to content

Conversation

@abhishekbedi1432
Copy link
Contributor

Motivation

Marathon for iOS produces an amazing HTML report (in the outputDir) but sometimes we need the final xcresult bundle for many reasons (say integrating with the CI, uploading the result in repository, etc).

The xcresult bundles are stored at transient paths under Xcode's derived-data folder.

In this PR, we have added an enhancement of providing xcResultBundlePath parameter in the Marathon file (under iOS Vendor Config)

As a positive side effect, this helps to avoid flooding Xcode's derived data directory.

abhishekbedi1432 and others added 8 commits September 17, 2022 01:02
…tag-xcxresult-testplan-from-anton

* ios-uitest-runner-via-tags:
  Update README.md
  Added Support to run iOS Test Runners Tag

# Conflicts:
#	configuration/src/main/kotlin/com/malinskiy/marathon/config/vendor/VendorConfiguration.kt
#	sample/ios-app/Marathonfile
… into develop/talabat/final-tag-xcxresult-testplan-from-anton

* develop/ios-support-for-test-plan-from-xcresult-branch:
  Modified XCTestRun to adapt to new format of xctestrun ( supporting test plan)
Copy link
Member

@Malinskiy Malinskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic wise we're missing pulling back the xcresult back to the marathon's side


Please check the official [documentation](https://marathonlabs.github.io/marathon/) for installation, configuration and more

## [iOS Only] Added Support to consume tags for test functions under XCTestCase Subclasses
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?: fileListProvider
val resolvedResultBundlePath = marathonfileDir.resolve(configuration.vendorConfiguration.xcResultBundlePath)

// Adding support for Test Plan
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the comment. It doesn't make sense after we merge this.

.fileList(resolvedDerivedDataDir)
.firstOrNull { it.extension == "xctestrun" }
?: throw ConfigurationException("Unable to find an xctestrun file in derived data folder")
.firstOrNull { it.extension == "xctestrun" && it.name.contains("$testPlanName") } ?: throw ConfigurationException("Unable to find matching TestPlan. Please recheck if testplan is enabled")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can skip creating an additional string here:
it.name.contains("$testPlanName") -> it.name.contains(testPlanName)
It might also be a good idea to use startsWith or equals instead of contains. I'm not sure if there is a contract on the file name here

val tests = applyTestFilters(parsedTests)
val shard = prepareTestShard(tests, analytics)

log.info("\n\n\n **** Marathon File Params **** \n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a no-no: configuration might contain sensitive credentials, and we can't print them to the stdout

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you really want to do this then we need to implement some kind of masking for sensitive fields

val method: String,
val metaProperties: Collection<MetaProperty>
val metaProperties: Collection<MetaProperty>,
val tags: Collection<String> = emptyList()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MetaProprety is an abstraction over tags, annotations and so on. We should just reuse MetaProperty here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refer:
[iOS]Added Support for Cucumberish Tags / Function Annotations #702

return tests
val iosConfig = configuration.vendorConfiguration as? VendorConfiguration.IOSConfiguration

return tests.filter { it.tags.contains(iosConfig?.xcTestRunnerTag) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just be an annotation filter, no vendor-specific logic in the core

super.tearDown()
}

// @Flowers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of modifying existing tests we should just add more tests with these tags

private fun TestBatch.toXcodebuildArguments(): String =
tests.joinToString(separator = " ") { "-only-testing:\"${it.pkg}/${it.clazz}/${it.method}\"" }

private fun getTimezone(): String {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method sounds like getTimestamp rather than getTimezone

override suspend fun extract(): List<Test> {

if (vendorConfiguration.xcTestRunnerTag.isNullOrEmpty()) {
logger.warn { "[iOS] Did not find `xcTestRunnerTag` in iOS Vendor Config. This will result in running all tests" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tag filtering doesn't have anything to do with the responsibility of the parser, this probably doesn't belong here

*/

private val targetMap: Map<String, *> = (
((((((propertyList.valueForKeypath("TestConfigurations") as? Array<Any>)?.first()) as Map<*, *>)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first() will throw NullPointerException here when TestConfigurations is missing. Reproduced on the old sample

@Malinskiy
Copy link
Member

Closes #620

@abhishekbedi1432
Copy link
Contributor Author

Pls refer: #700 for the feature (Adding support for XCResult). I will close this PR addressing all the comments in #700

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants