Skip to content
This repository has been archived by the owner on Dec 7, 2019. It is now read-only.

[WIP] "Orchestrator like" execution mode #138

Merged
merged 12 commits into from
Apr 25, 2018
Merged

[WIP] "Orchestrator like" execution mode #138

merged 12 commits into from
Apr 25, 2018

Conversation

tagantroy
Copy link
Contributor

@tagantroy tagantroy commented Apr 20, 2018

#135 implementation

  • Add dependency
  • Implement test parser
  • add buildSingleTestArguments

.value()

fun parseTests(testApkPath: String) = DexParser.findTestMethods(testApkPath)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please specify return type explicitly here?

@@ -3,6 +3,9 @@ package com.gojuno.composer
import com.gojuno.commander.android.aapt
import com.gojuno.commander.os.Notification
import com.gojuno.commander.os.process
import com.linkedin.dex.parser.DexParser
import com.linkedin.dex.parser.TestMethod
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks unused :)

@artem-zinnatullin
Copy link
Collaborator

@dsvoronin PTAL

fun parseTests(testApkPath: String) : List<TestMethod> =
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can add an integration test (we already have some that parse runner from test apk, you can add couple tests there)

Not that I don't trust this library, but it can help catch future regressions!

@tagantroy
Copy link
Contributor Author

@artem-zinnatullin
what do you think about 2 strategies:

  1. execute all test on each device
  2. all devices poll from common queue

.value()

fun parseTests(testApkPath: String) : List<String> =
Copy link
Contributor

@dsvoronin dsvoronin Apr 21, 2018

Choose a reason for hiding this comment

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

Seems like we need to keep TestMethod, to filter out @Ignore before run and keep custom annotations info

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally we'd have own TestMethod objects (we have some, I'm not sure they cover this use-case), so plugins (see below) could extend it with custom info and Dexparser lib would be just implementation detail rather than API everyone has to use

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, i'm all into this kind of abstraction

.value()

fun parseTests(testApkPath: String) : List<String> =
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we need handling of our own annotations to modify resulting junit report. How this can be possible without forking composer? Any plugin mechanism?

Copy link
Contributor

Choose a reason for hiding this comment

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

answering my own question: probably we need something like this

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, we discussed plugins before, it shouldn't be super hard since we're running Composer on JVM. We can add a parameter to load plugin(s) as jars and can add abstractions and APIs to hook into

@dsvoronin can you please create a separate issue for Plugin System?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dsvoronin
Copy link
Contributor

dsvoronin commented Apr 21, 2018

what do you think about 2 strategies:

  1. execute all test on each device
  2. all devices poll from common queue

I'm not Artem :) , but we need at least all devices poll from common queue strategy. We spawn N exact clones, and they polls from queue.

We definitely need abstraction for different strategies here

@artem-zinnatullin
Copy link
Collaborator

I'd start with just integrating Orchestrator-like process isolation into current shart true/false mode

Dynamic test sharding is just something we can build on top of test parsing, we should allow running dynamically sharded tests without "Orchestrator" right

@yunikkk
Copy link
Contributor

yunikkk commented Apr 23, 2018

+1 for polling from the main queue and having in mind other strategies in future.

@@ -198,6 +198,11 @@ private fun List<String>.pairArguments(): List<Pair<String, String>> =
}
}


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove extra line above/below function

@@ -18,5 +18,8 @@ class ApkSpec : Spek({
it("parses test package correctly") {
assertThat(parseTestPackage(testApkPath)).isEqualTo(TestPackage.Valid("test.test.myapplication.test"))
}
it("parses tests list correctly") {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra line?

@artem-zinnatullin
Copy link
Collaborator

LGTM, let's merge as first part?

@yunikkk
Copy link
Contributor

yunikkk commented Apr 24, 2018

@artem-zinnatullin yup, the only question is unchecked "Execute list of tests", @tagantroy are you going to add smth here?

@tagantroy
Copy link
Contributor Author

@artem-zinnatullin @yunikkk let's merge it

@yunikkk yunikkk merged commit c4e7668 into gojuno:master Apr 25, 2018
@yunikkk
Copy link
Contributor

yunikkk commented Apr 25, 2018

@tagantroy merged

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

Successfully merging this pull request may close these issues.

None yet

4 participants