Skip to content

LOG-1028: Rewrite e2e test in go #643

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 26, 2021
Merged

LOG-1028: Rewrite e2e test in go #643

merged 1 commit into from
Feb 26, 2021

Conversation

huikang
Copy link
Contributor

@huikang huikang commented Feb 10, 2021

Description

Rewrite e2e test in go

/cc @ewolinetz @periklis
/assign @ewolinetz

Links

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 10, 2021
@huikang huikang force-pushed the LOG-1028-go-test branch 2 times, most recently from def7ee3 to 2bb8a30 Compare February 10, 2021 19:37
@huikang huikang changed the title [WIP] LOG-1028: Rewrite e2e test in go LOG-1028: Rewrite e2e test in go Feb 10, 2021
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 10, 2021
periklis
periklis reviewed Feb 15, 2021
View reviewed changes
Copy link
Contributor

@periklis periklis left a comment

Choose a reason for hiding this comment

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

In general it looks good to me. One question though, what is the purpose to keep the different bash-files as drivers of the golang tests?

test/e2e-olm/elasticsearch_metric_test.go
Comment on lines 121 to 120
if secret == nil {

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing block 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.

oops. Will fix. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@huikang
Copy link
Contributor Author

huikang commented Feb 15, 2021

In general it looks good to me. One question though, what is the purpose to keep the different bash-files as drivers of the golang tests?

Thanks for asking this question, @periklis ! I was trying to consolidate them into a single place. However, it is more complicated than I thought due to how the bash scripts and functions written in bash are organized.
Therefore, I avoided making drastic change to those bash files.
I think cleaning up the bash script of driving the e2e test probably worth a 1 or 2 point jira task by its own.

@huikang huikang force-pushed the LOG-1028-go-test branch 2 times, most recently from aaebe0f to 99ba968 Compare February 16, 2021 02:09
periklis
periklis reviewed Feb 16, 2021
View reviewed changes
@huikang huikang force-pushed the LOG-1028-go-test branch 3 times, most recently from 12f2bed to d8d691d Compare February 16, 2021 17:55
periklis
periklis suggested changes Feb 17, 2021
View reviewed changes
Copy link
Contributor

@periklis periklis left a comment

Choose a reason for hiding this comment

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

Looks overall good to me. However, I noticed that the tests are missing in the junit report:
image

hack/testing-olm/test-657-im-block-autocreate-for-write-suffix.sh Outdated
os::log::info "---------------------------------------------------------------"
os::cmd::expect_success_and_text "oc -n $TEST_NAMESPACE exec $pod -c elasticsearch -- es_util --query=foo-write/_doc/1 -d '{\"key\":\"value\"}' -XPUT -w %{http_code}" ".*201"
TEST_WATCH_NAMESPACE=${TEST_NAMESPACE} TEST_OPERATOR_NAMESPACE=${TEST_NAMESPACE} \
go test -v ./test/e2e-olm/... -kubeconfig=${KUBECONFIG} -parallel=1 -timeout 1500s -run TestElasticsearchWrite
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing passing this test to the junit report like here

Copy link
Contributor Author

@huikang huikang Feb 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed. Thanks!

hack/testing-olm/test-200-verify-es-metrics-access.sh Outdated
CLUSTERROLE=${CLUSTERROLE} AUTHORIZED_SA=${AUTHORIZED_SA} UNAUTHORIZED_SA=${UNAUTHORIZED_SA} \
TEST_OPERATOR_NAMESPACE=${TEST_NAMESPACE} \
TEST_WATCH_NAMESPACE=${TEST_NAMESPACE} \
go test -v ./test/e2e-olm/... -kubeconfig=${KUBECONFIG} -parallel=1 -timeout 1500s -run TestElasticsearchOperatorMetrics
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing passing this test to the junit report like 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.

Fixed. Thanks!

@periklis
Copy link
Contributor

Great job! PTAL a look why the junit report includes the two new tests twice. Do we really run them twice?
image

@huikang
Copy link
Contributor Author

huikang commented Feb 18, 2021

@periklis , I think the first one of the parent test name of the second one. I checked the results of other test file and find that they have similar structure.

Screen Shot 2021-02-18 at 10 49 22 AM

To avoid the confusion, how about change the test name like

TestElasticsearchOperatormetrics / ReadMetrics

TestElasticssearchWrite/ WriteDocs

What do you think?

@periklis
Copy link
Contributor

Renaming sounds good. However, my point is that if you check the junit report listing you will notice that the following tests are listed twice:

e2e-olm: TestElasticsearchOperatorMetrics
e2e-olm: TestElasticsearchOperatorMetrics/Operator_metrics
e2e-olm: TestElasticsearchWrite
e2e-olm: TestElasticsearchWrite/elasticsearch_write

ewolinetz
ewolinetz reviewed Feb 19, 2021
View reviewed changes
hack/testing-olm/test-200-verify-es-metrics-access.sh
# allowed to retrieve metrices from elasticsearch
set -euo pipefail

KUBECONFIG=${KUBECONFIG:-$HOME/.kube/config}
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to have the KUBECONFIG here? the tests worked correctly before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we need KUBECONFIG here because the new operator sdk has removed its test framework, so we need to set KUBECONFIG for the go test command. Actually, we have done similar change to test-001:

elasticsearch-operator/hack/testing-olm/test-001-operator-sdk-e2e.sh

Line 4 in 909c5a4

KUBECONFIG=${KUBECONFIG:-$HOME/.kube/config}

@ewolinetz
Copy link
Contributor

one question about the KUBECONFIG variable.. otherwise looks good
/approve

@ewolinetz
Copy link
Contributor

please also squash your commits @huikang

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 19, 2021
@huikang
Copy link
Contributor Author

huikang commented Feb 20, 2021

TestElasticsearchWrite

Renaming sounds good. However, my point is that if you check the junit report listing you will notice that the following tests are listed twice:

e2e-olm: TestElasticsearchOperatorMetrics
e2e-olm: TestElasticsearchOperatorMetrics/Operator_metrics
e2e-olm: TestElasticsearchWrite
e2e-olm: TestElasticsearchWrite/elasticsearch_write

Hi, @periklis , it looks all other tests (like TestElasticsearchCluster/Invalid_master_count) appear twice in the junit page (https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_elasticsearch-operator/643/pull-ci-openshift-elasticsearch-operator-master-e2e-operator/1362067388459126784). It shows 2 matches found for each test case.

@huikang
Copy link
Contributor Author

huikang commented Feb 21, 2021

retest

@huikang
Copy link
Contributor Author

huikang commented Feb 22, 2021

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

28 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 84a0d78 into openshift:master Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@ewolinetz ewolinetz ewolinetz left review comments

@periklis periklis periklis approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@periklis periklis

@ewolinetz ewolinetz

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@huikang @periklis @ewolinetz @openshift-ci-robot @openshift-bot @openshift-merge-robot