Skip to content
This repository was archived by the owner on Jan 4, 2026. It is now read-only.

Refactor the test name uploaded to ES#35

Open
alejandromontero wants to merge 1 commit into
masterfrom
Refactor/dynamic-test-name-yarn
Open

Refactor the test name uploaded to ES#35
alejandromontero wants to merge 1 commit into
masterfrom
Refactor/dynamic-test-name-yarn

Conversation

@alejandromontero
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

@fenech fenech left a comment

Choose a reason for hiding this comment

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

It would also be good to add some extra tests to show the expected behaviour when application tags has zero or one elements.

Comment thread shell/common/common_hive.sh Outdated
PARQUET_COMPRESSION "$PARQUET_COMPRESSION" \
ORC_COMPRESSION "$ORC_COMPRESSION" \
EXPERIMENT_ID "$EXPERIMENT_ID"
EXPERIMENT_ID "$EXPERIMENT_ID,$1"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This isn't the experiment ID any more - I would suggest changing the template variable to ##APPLICATION_TAGS## and then the second argument on this line would be something like "$EXPERIMENT_ID,$display_name".

Comment thread shell/common/common_hive.sh Outdated

$DSH "$(get_perl_exports)
/usr/bin/perl -i -pe '$(get_hive_substitutions)' '$HIVE_SETTINGS_FILE' '$(get_hive_conf_dir)'/*"
/usr/bin/perl -i -pe '$(get_hive_substitutions $1)' '$HIVE_SETTINGS_FILE' '$(get_hive_conf_dir)'/*"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing quotes around '$1' - since this is a human-readable "display name", we should support writing it however we want.

Comment thread shell/common/common_spark.sh Outdated
SPARK_MEMORY_OVERHEAD "$SPARK_MEMORY_OVERHEAD" \
EXECUTOR_MEM "$EXECUTOR_MEM" \
EXPERIMENT_ID "$EXPERIMENT_ID"
EXPERIMENT_ID "$EXPERIMENT_ID,$1"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As above, change the name of the template var.

Comment thread shell/common/common_spark.sh Outdated
echo -e "$(get_local_bench_path)/spark_conf"
}

# $1: Benchmark name
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not completely against these types of "documentation comments" but we can also make the code clearer by assigning to a local variable display_name=$1.

Comment thread shell/test/parse_yarn_output.bats Outdated
}
@test "sets the expected experimentId" {
run jq -r '.experimentId' <<<"${lines[1]}"
[ $output = 'test' ]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would put quotes around $output, since it can be any string (we know that $status must be an integer, so not necessary there, but if you prefer to add them to be consistent then that's fine).

Comment thread shell/test/parse_yarn_output.bats Outdated
@test "sets the expected displayName" {
run jq -r '.displayName' <<<"${lines[1]}"
[ $output = 'populatemetastore' ]
} No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing newline at end of file and quotes again, please.

@alejandromontero alejandromontero force-pushed the Refactor/dynamic-test-name-yarn branch from 1322a57 to eae89f5 Compare November 20, 2017 09:44
@alejandromontero alejandromontero force-pushed the Refactor/dynamic-test-name-yarn branch from eae89f5 to 40b1857 Compare November 20, 2017 09:46
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.

2 participants