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

[BEAM-3339] Mobile gaming automation for Java nightly snapshot on core runners #4788

Merged
merged 1 commit into from Apr 19, 2018

Conversation

Projects
None yet
4 participants
@yifanzou
Contributor

yifanzou commented Mar 2, 2018

DESCRIPTION HERE

In this change, we added test automation of mobile-gaming (UserScore, HourlyTeamScore, LeaderBoard) on Direct and Dataflow running against nightly snapshot.


Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue.
  • Write a pull request description that is detailed enough to understand:
    • What the pull request does
    • Why it does it
    • How it does it
    • Why this approach
  • Each commit in the pull request should have a meaningful subject line and body.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@yifanzou yifanzou changed the title from BEAM-3339 Mobile gaming automation for Java nightly snapshot -- testing to [BEAM-3339] Mobile gaming automation for Java nightly snapshot on core runners Mar 9, 2018

@yifanzou

This comment has been minimized.

Contributor

yifanzou commented Mar 9, 2018

@yifanzou

This comment has been minimized.

Contributor

yifanzou commented Mar 9, 2018

+R: @lukecwik

@yifanzou

This comment has been minimized.

Contributor

yifanzou commented Mar 13, 2018

@lukecwik This is the code change of java mobile-gaming automation on core runners (direct, dataflow). Please review it when you get a moment. Thanks~

@yifanzou

This comment has been minimized.

Contributor

yifanzou commented Mar 14, 2018

+R: @tgroh

// runner [Direct, Dataflow, Spark, Flink, FlinkLocal, Apex]
String runner
// gcpProject sets the gcpProject argument when executing examples.

This comment has been minimized.

@tgroh

tgroh Mar 15, 2018

Member

Is this required for all of the examples?

Certainly the new ones look as though they're only appropriate for the MobileGaming examples. Can we fork at least some parts of the two configs?

This comment has been minimized.

@yifanzou

yifanzou Mar 16, 2018

Contributor

They are used by both Quickstart and MobileGaming except for "dataset", which is MobileGaming specific.

"type", "runner" -- required;
"gcpProject", "gcsBucket" -- used by examples cooperate with gcloud;
"pubsubTopic" -- required by MobileGaming at this moment, but it's also good to have it since we are going to automate streaming wordcount examples in the future.

}
private static String generateLeaderBoardCommand(String runner, TestScripts t){

This comment has been minimized.

@tgroh

tgroh Mar 15, 2018

Member

Is there some subset of default args that can be populated in advance that we're sure we'll need for all of the xamples (runner, project, etc)

def LeaderBoardThread = Thread.start() {
t.run(MobileGamingJavaUtils.generateCommand("LeaderBoard", "DirectRunner",t))
}
// wait 15 minutes for pipeline writing results

This comment has been minimized.

@tgroh

tgroh Mar 15, 2018

Member

I'm not super happy about having a correctness test that requires fifteen minutes, nor one that doesn't listen to some signal from the injector or pipeline to note that 'everything is done'.

Even if this is how we move forwards for the time being, I'd like to see that this is tracked to be more event-driven; we can expect to have a timeout, but that's not really the same.

t.run "gsutil cat gs://${t.gcsBucket()}/java-hourlyteamscore-result-dataflow-runner* | grep AzureBilby "
t.see "total_score: 2788, team: AzureBilby"
t.intent("SUCCEED: HourlyTeamScore successfully run on DataflowRunner.")
t.run "gsutil rm gs://${t.gcsBucket()}/java-hourlyteamscore-result-dataflow-runner*"

This comment has been minimized.

@tgroh

tgroh Mar 15, 2018

Member

Can we configure these patterns to be identified per-run?

Is it possible to configure the testing buckets to also have a TTL, in case we write the files but then the test dies? Do we already have such a configuration?

t.see "leaderboard_DataflowRunner_user"
t.see "leaderboard_DataflowRunner_team"
t.run """bq query --batch "SELECT user FROM [${t.gcpProject()}:${t.bqDataset()}.leaderboard_DataflowRunner_user] LIMIT 10\""""
t.seeOneOf(MobileGamingJavaUtils.COLORS)

This comment has been minimized.

@tgroh

tgroh Mar 15, 2018

Member

I'd really like to be able to configure the injector to use a constant (but arbitrary) seed which we can match actual outputs against the expected generation (for at least on-time work)

@@ -518,24 +518,33 @@ ext.applyAvroNature = {
// A class defining the set of configurable properties for createJavaQuickstartValidationTask
class JavaQuickstartConfiguration {

This comment has been minimized.

@lukecwik

lukecwik Mar 19, 2018

Member

rename to JavaExamplesArchetypeValidationConfiguration?

This comment has been minimized.

@yifanzou

yifanzou Mar 21, 2018

Contributor

done

String bqDataset
// pubsubTopic sets topics when executing streaming pipelines
String pubsubTopic
}
// Creates a task to run the quickstart for a runner.
// Releases version and URL, can be overriden for a RC release with
// ./gradlew :release:runQuickstartJava -Pver=2.3.0 -Prepourl=https://repository.apache.org/content/repositories/orgapachebeam-1027
ext.createJavaQuickstartValidationTask = {

This comment has been minimized.

@lukecwik

lukecwik Mar 19, 2018

Member

rename to createJavaExamplesArchetypeValidationTask?

This comment has been minimized.

@yifanzou

yifanzou Mar 21, 2018

Contributor

done

StringBuilder exeArgs = new StringBuilder(commonArgs)
exeArgs.append("--input=gs://${t.gcsBucket()}/5000_gaming_data.csv ")
if(runner == "DataflowRunner"){

This comment has been minimized.

@lukecwik

lukecwik Mar 19, 2018

Member

nit: whitespace

here and elsewhere

if(runner == "DataflowRunner"){
exeArgs.append("--project=${t.gcpProject()} ")
.append("--output=gs://${t.gcsBucket()}/${getUserScoreOutputName(runner)} ")
}

This comment has been minimized.

@lukecwik

lukecwik Mar 19, 2018

Member

nit: style
} else {

here and elsewhere

This comment has been minimized.

@yifanzou

yifanzou Mar 21, 2018

Contributor

done

class MobileGamingJavaUtils {
public static final RUNNERS = [DirectRunner: "direct-runner",

This comment has been minimized.

@lukecwik

lukecwik Mar 19, 2018

Member

nit: indentation is 2 spaces

This comment has been minimized.

@yifanzou

yifanzou Mar 21, 2018

Contributor

done.

cmd.append("mvn compile exec:java -q ")
.append("-Dexec.mainClass=org.apache.beam.examples.complete.game.UserScore ")
.append("-Dexec.args=\"${exeArgs.toString()}\" ")

This comment has been minimized.

@lukecwik

lukecwik Mar 19, 2018

Member

isn't the .toString() implicit already?

here and below

This comment has been minimized.

@yifanzou

yifanzou Mar 21, 2018

Contributor

You're right. toString() removed.

@yifanzou

This comment has been minimized.

Contributor

yifanzou commented Mar 21, 2018

@lukecwik Changes are made base on comments. Please review. Thanks.

  1. rename task names and related methods.
  2. re-formatting code follows google styles.
public static final EXECUTION_TIMEOUT = 20
// Lists used to verify team names generated in the LeaderBoard example

This comment has been minimized.

@tgroh

tgroh Mar 21, 2018

Member

Does this needs to be kept in sync with the injector, if that ever changes? If so, there should also be a comment there

public static String generateCommand(String exampleName, String runner, TestScripts t){
String commonArgs = "--tempLocation=gs://${t.gcsBucket()}/tmp --runner=${runner} "
if(exampleName.equals("UserScore")){

This comment has been minimized.

@tgroh

tgroh Mar 21, 2018

Member

Can we use a switch?

}
private static String generateUserScoreCommand(String runner, TestScripts t, String commonArgs){

This comment has been minimized.

@tgroh

tgroh Mar 21, 2018

Member

I find the use of commonArgs as a parameter a bit skeevy - perhaps a method to generate them instead?

It's especially true as the cmd is built basically the same way in four places - add a similar method for configuring that command?

StringBuilder cmd = new StringBuilder()
StringBuilder exeArgs = new StringBuilder(commonArgs)
exeArgs.append("--project=${t.gcpProject()} ")

This comment has been minimized.

@tgroh

tgroh Mar 21, 2018

Member

is there any way to take a varargs (or an iterable of args, or probably most preferably a map or multimap) and build this string from that configuration? Making it a map literal seems like it might read more easily.

You could combine that with a method to generate the exec command for these examples, and significantly reduce the amount of string-builder specs you have in these methods.

// Check for expected results in stdout of the last command
// Check for expected results in stdout of the last command, return boolean
public boolean seeKeyWord(String expected){
if (!var.lastText.contains(expected)){

This comment has been minimized.

@tgroh

tgroh Mar 21, 2018

Member

This doesn't really do anything as written. Even if you want to inject a predicate,

Even if it is important to do it with a method, just return var.lastText.contains(expected)

@yifanzou

This comment has been minimized.

Contributor

yifanzou commented Mar 22, 2018

Changes are made:

  1. add comments in Injector that indicate the team list should sync to the list in MobileGamingJavaUtils.
  2. Use map to create arguments for each examples.
  3. Rewrite command generation method in MobileGamingJavaUtils.
  4. Add "-q" option for each quickstart command that stop writing [Info] messages into console during maven compile. This make the console output cleaner.
    PTAL @tgroh
@@ -0,0 +1,148 @@
#!groovy
import java.text.SimpleDateFormat

This comment has been minimized.

@tgroh

tgroh Mar 23, 2018

Member

This ordering seems off - unless there's a traditional style I'm missing

ApexRunner: "apex-runner",
FlinkRunner: "flink-runner"]
public static final EXECUTION_TIMEOUT = 20

This comment has been minimized.

@tgroh

tgroh Mar 23, 2018

Member

Unitless values aren't great

This comment has been minimized.

@yifanzou

yifanzou Mar 27, 2018

Contributor

EXECUTION_TIMEOUT -> EXECUTION_TIMEOUT_IN_MINUTES

"BarnRed",
"BattleshipGrey"))
private static final USERSCORE_OUTPUT_PREFIX = "java-userscore-result-"

This comment has been minimized.

@tgroh

tgroh Mar 23, 2018

Member

This should be inlineable

This comment has been minimized.

@yifanzou

yifanzou Mar 27, 2018

Contributor

done

private static final USERSCORE_OUTPUT_PREFIX = "java-userscore-result-"
private static final HOURLYTEAMSCORE_OUTPUT_PREFIX = "java-hourlyteamscore-result-"

This comment has been minimized.

@tgroh

tgroh Mar 23, 2018

Member

Same.

This comment has been minimized.

@yifanzou

yifanzou Mar 27, 2018

Contributor

done

}
println "Verified $expected"
}
public boolean seeOneOf(List<String> expected) {

This comment has been minimized.

@tgroh

tgroh Mar 23, 2018

Member

seeAnyOf? This succeeds on 1 or more matches

This comment has been minimized.

@yifanzou

yifanzou Mar 27, 2018

Contributor

done.

return false
}
public boolean seeExact(String expected) {

This comment has been minimized.

@tgroh

tgroh Mar 23, 2018

Member

Do we use this anywhere?

Even if we do, it looks more like "seeSubstring"

This comment has been minimized.

@yifanzou

yifanzou Mar 27, 2018

Contributor

yes, we do need it while checking whether tables are created/delete from dataset.
Renamed method being seeSubstring()

t.run(MobileGamingJavaUtils.createExampleExecutionCommand("UserScore", runner, t))
t.run "gsutil cat gs://${t.gcsBucket()}/${MobileGamingJavaUtils.getUserScoreOutputName(runner)}* | grep user19_BananaWallaby"
t.see "total_score: 231, user: user19_BananaWallaby"
t.intent("SUCCEED: UserScore successfully run on DataflowRunner.")

This comment has been minimized.

@tgroh

tgroh Mar 23, 2018

Member

This doesn't seem like an intent to me

This comment has been minimized.

@yifanzou

yifanzou Mar 27, 2018

Contributor

Made a new method success() in TestScripts

* Run the UserScore example on DataflowRunner
* */
t.intent("Running: UserScore example on DataflowRunner")

This comment has been minimized.

@tgroh

tgroh Mar 23, 2018

Member

How do multiple intents interact with each other?

This comment has been minimized.

@yifanzou

yifanzou Mar 27, 2018

Contributor

Using intents to document intentions of each example run. Using "describe()" at very begging of the code to document the purpose of the script.

QuickstartArchetype.generate(t)
t.intent 'Running the Mobile-Gaming Code with DataflowRunner'

This comment has been minimized.

@tgroh

tgroh Mar 23, 2018

Member

Some of the t.intent calls are formatted inconsistently with the others.

This comment has been minimized.

@yifanzou

yifanzou Mar 27, 2018

Contributor

done

// it will take couple seconds to clean up tables. This loop makes sure tables are completely deleted before running the pipeline
while({
sleep(3000)
t.run ("bq query SELECT table_id FROM ${t.bqDataset()}.__TABLES_SUMMARY__")

This comment has been minimized.

@tgroh

tgroh Mar 23, 2018

Member

I'm not super happy about the mutation of the TestScripts object to check a state - can't it just return the content of interest?

@yifanzou

This comment has been minimized.

Contributor

yifanzou commented Mar 29, 2018

PTAL @tgroh

createJavaExamplesArchetypeValidationTask(type: 'Quickstart', runner: 'Direct')
// Generates :runners:direct-java:runMobileGamingJavaDirect
createJavaExamplesArchetypeValidationTask(type: 'MobileGaming',

This comment has been minimized.

@tgroh

tgroh Apr 4, 2018

Member

I fear that this may choke without restricting the scope of the input, as these are moderately sized files. Have we run these tasks locally (with potentially modified staging, output, projects?)

This comment has been minimized.

@yifanzou

yifanzou Apr 4, 2018

Contributor

discussed offline.

@tgroh

Rebase on top of the gradle updates?

@tgroh

tgroh approved these changes Apr 9, 2018

Minor nits and style changes, otherwise I'm happy

return "java-hourlyteamscore-result-${RUNNERS[runner]}.txt"
}
public static String createExampleExecutionCommand(String exampleName, String runner, TestScripts t){

This comment has been minimized.

@tgroh

tgroh Apr 9, 2018

Member

createPipelineCommand

return "java-userscore-result-${RUNNERS[runner]}.txt"
}
public static String getHourlyTeamScoreOutputName(String runner){

This comment has been minimized.

@tgroh

tgroh Apr 9, 2018

Member

If these are sharded (which I imagine is the default behavior), is this just the base pattern?

}
private static Map getHourlyTeamScoreArgs(String runner, TestScripts t){
if(runner == "DataflowRunner"){

This comment has been minimized.

@tgroh

tgroh Apr 9, 2018

Member

Is there a default formatter? Some of these formatting styles are a bit unfamiliar - but specifically the way you're breaking arrays that would go past the line limit seem like they should break on every arg. If there's a guide you're following, though that's cool

break
}
}
println "Waiting for verifying results..."

This comment has been minimized.

@tgroh

tgroh Apr 9, 2018

Member

Something more like "Waiting for pipeline to produce more results"?

-P${RUNNERS[runner]}"""
}
public static String createInjectorCommand(TestScripts t){

This comment has been minimized.

@tgroh

tgroh Apr 9, 2018

Member

I'm not a huge fan of how every one of the commands to create a command or args takes TestScripts - perhaps this should be an object that contains it as a field?

It shouldn't even be too difficult to instantiate -
new MobileGamingCommands(testScripts: t)

(If you do, rename as above)

@yifanzou

This comment has been minimized.

Contributor

yifanzou commented Apr 10, 2018

Run Seed Job

@yifanzou

This comment has been minimized.

Contributor

yifanzou commented Apr 10, 2018

Run Dataflow PostRelease

@yifanzou

This comment has been minimized.

Contributor

yifanzou commented Apr 11, 2018

@tgroh I am appreciate for your comments and helps on this. I made some changes base on previous comments, rebased the HEAD and squashed commits. PTAL

@tgroh

This comment has been minimized.

Member

tgroh commented Apr 12, 2018

run java precommit

1 similar comment
@yifanzou

This comment has been minimized.

Contributor

yifanzou commented Apr 13, 2018

run java precommit

@robertwb

This comment has been minimized.

Contributor

robertwb commented Apr 13, 2018

Jenkins: retest this please.

@yifanzou

This comment has been minimized.

Contributor

yifanzou commented Apr 14, 2018

@tgroh Please merge if you are happy with the code change :) Thanks!

@tgroh tgroh merged commit 7300eb2 into apache:master Apr 19, 2018

2 of 3 checks passed

Jenkins: ./gradlew --info --continue --rerun-tasks --no-daemon :pythonPreCommit FAILURE
Details
Jenkins: ./gradlew --info --continue --rerun-tasks --no-daemon :goPreCommit SUCCESS
Details
Jenkins: ./gradlew :javaPreCommit SUCCESS
Details

@yifanzou yifanzou deleted the yifanzou:BEAM-3339/java-mobile-gaming-automation-nightly-snapshots branch Apr 19, 2018

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