Skip to content
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

chore: Improve integ test wrapper script #13628

Merged
merged 4 commits into from
Aug 24, 2022

Conversation

vktng
Copy link
Contributor

@vktng vktng commented Aug 16, 2022

Summary

The integration test wrapper script for Bazel was lacking some desired features, this PR aims to resolve 4 of them:

  1. The option --retry-on-failure and --retry-attempts N was added. With these you can rerun failing tests N times and they will only be considered failed if all attempts fail.
  2. Multiple test targets can be specified. You can provide your own list of targets that can contain tests from all categories and the order does not matter.
  3. After every run there will be a Junit XML report generated.
  4. With the --rerun-previously-failed option you can rerun all previously failed tests.

Test Plan

Use options as described in bazel/scripts/run_integ_tests.sh --help.

Some examples:

  • Quick, only precommit tests:
bazel/scripts/run_integ_tests.sh  //lte/gateway/python/integ_tests/s1aptests:test_attach_detach //lte/gateway/python/integ_tests/s1aptests:test_attach_detach_static_ip //lte/gateway/python/integ_tests/s1aptests:test_gateway_metrics_attach_detach //lte/gateway/python/integ_tests/s1aptests:test_attach_without_ips_available
  • More flaky and longer nonsanity tests:
bazel/scripts/run_integ_tests.sh  //lte/gateway/python/integ_tests/s1aptests:test_outoforder_erab_setup_rsp_dedicated_bearer  //lte/gateway/python/integ_tests/s1aptests:test_attach_detach_multi_ue_looped //lte/gateway/python/integ_tests/s1aptests:test_attach_detach_nw_triggered_delete_secondary_pdn //lte/gateway/python/integ_tests/s1aptests:test_attach_detach_ipv6
  • Retry:
bazel/scripts/run_integ_tests.sh --retry-on-failure //lte/gateway/python/integ_tests/s1aptests:test_outoforder_erab_setup_rsp_dedicated_bearer --retry-attempts 1 //lte/gateway/python/integ_tests/s1aptests:test_attach_detach_multi_ue_looped //lte/gateway/python/integ_tests/s1aptests:test_attach_detach_nw_triggered_delete_secondary_pdn //lte/gateway/python/integ_tests/s1aptests:test_attach_detach_ipv6
  • Rerun:
bazel/scripts/run_integ_tests.sh  --rerun-previously-failed

Additional Information

  • This change is backwards-breaking

@vktng vktng added the bazel changes for the Bazelification effort label Aug 16, 2022
@vktng vktng self-assigned this Aug 16, 2022
@vktng vktng requested a review from a team August 16, 2022 16:53
@vktng vktng requested a review from a team as a code owner August 16, 2022 16:53
@vktng vktng requested a review from jheidbrink August 16, 2022 16:53
@pull-request-size pull-request-size bot added the size/L Denotes a Pull Request that changes 100-499 lines. label Aug 16, 2022
@vktng vktng requested a review from ajahl August 16, 2022 16:53
@github-actions
Copy link
Contributor

Thanks for opening a PR! 💯

A couple initial guidelines

Howto

  • Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
  • Checks. All required CI checks must pass before merge.
  • Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

If this is your first Magma PR, also consider reading

@github-actions github-actions bot added the component: agw Access gateway-related issue label Aug 16, 2022
bazel/scripts/run_integ_tests.sh Outdated Show resolved Hide resolved
bazel/scripts/run_integ_tests.sh Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Aug 16, 2022

feg-workflow

    2 files  203 suites   40s ⏱️
374 tests 374 ✔️ 0 💤 0
388 runs  388 ✔️ 0 💤 0

Results for commit fee0c71.

♻️ This comment has been updated with latest results.

@vktng vktng force-pushed the improve_integ_test_wrapper_script branch from bb870b9 to 9dde152 Compare August 16, 2022 17:12
@github-actions
Copy link
Contributor

github-actions bot commented Aug 16, 2022

dp-workflow

13 tests   13 ✔️  2m 9s ⏱️
  1 suites    0 💤
  1 files      0

Results for commit fee0c71.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 16, 2022

agw-workflow

615 tests   611 ✔️  5m 21s ⏱️
    2 suites      4 💤
    2 files        0

Results for commit c7c861a.

♻️ This comment has been updated with latest results.

@vktng vktng linked an issue Aug 17, 2022 that may be closed by this pull request
@vktng vktng requested a review from ardzoht August 17, 2022 13:34
Copy link
Contributor

@nstng nstng left a comment

Choose a reason for hiding this comment

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

As discussed offline:

  • $ ./bazel/scripts/run_integ_tests.sh --retry-on-failure results in ./bazel/scripts/run_integ_tests.sh: line 111: POSITIONAL_ARGS: unbound variable
  • report/merged_report/report_all_tests.xml should not be localted under magma root, e.g., use something related to RESULTS_DIR := /var/tmp/test_results in make integ tests
    -> in the repo is not good
  • fix result xml - make it publishable

@vktng vktng force-pushed the improve_integ_test_wrapper_script branch from 9dde152 to fe00aee Compare August 22, 2022 14:53
@vktng vktng requested a review from nstng August 22, 2022 14:54
@LKreutzer LKreutzer removed the request for review from jheidbrink August 22, 2022 14:55
@vktng
Copy link
Contributor Author

vktng commented Aug 22, 2022

As discussed offline:

* `$ ./bazel/scripts/run_integ_tests.sh --retry-on-failure` results in `./bazel/scripts/run_integ_tests.sh: line 111: POSITIONAL_ARGS: unbound variable`

* `report/merged_report/report_all_tests.xml` should not be localted under magma root, e.g., use something related to `RESULTS_DIR := /var/tmp/test_results` in make integ tests
  -> in the repo is not good

* fix result xml - make it publishable

Fixed.

@vktng vktng removed the request for review from ardzoht August 22, 2022 14:57
@@ -46,12 +46,15 @@ def merge_all_report(working_dir, list_xml_report_paths, output_path):
xml_file_path = working_dir + "/" + xml_file_path
test_result = ET.parse(xml_file_path)
test_suites_data = test_result.getroot()

integration_tests = test_suites_data.attrib == {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused by the these changes. So if we're running integration tests, there's an empty dict here and the real data is in the first element? Maybe it would be good to add a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the xml is created with pytest then the structure is slightly different from what this script was intended to work with. If the attributes are missing, then we have to copy them from the child node.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. 👍

By the way, there's an unused variable in this function called c that you could remove if you like.

Copy link
Contributor

@sebathomas sebathomas left a comment

Choose a reason for hiding this comment

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

AGW changes looking good.

@@ -46,12 +46,15 @@ def merge_all_report(working_dir, list_xml_report_paths, output_path):
xml_file_path = working_dir + "/" + xml_file_path
test_result = ET.parse(xml_file_path)
test_suites_data = test_result.getroot()

integration_tests = test_suites_data.attrib == {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I see. 👍

By the way, there's an unused variable in this function called c that you could remove if you like.

@vktng vktng force-pushed the improve_integ_test_wrapper_script branch 3 times, most recently from 6a509f1 to c7c861a Compare August 23, 2022 10:07
@@ -1199,10 +1198,7 @@ pytest_test(
srcs = ["test_attach_detach_flaky_retry_success.py"],
imports = [LTE_ROOT],
tags = TAG_EXTENDED_TEST,
deps = [
":s1ap_wrapper",
requirement("flaky"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since "flaky" is a dependency of pytest from now on you do not need to specify it. (It will lead to a duplication error.)
See bazel/python_test.bzl

@sebathomas
Copy link
Contributor

Needs to be rebased to get the fix for the DP CI issue from master.

Signed-off-by: Krisztián Varga <krisztian.varga@tngtech.com>
…t wrapper script

Signed-off-by: Krisztián Varga <krisztian.varga@tngtech.com>
Signed-off-by: Krisztián Varga <krisztian.varga@tngtech.com>
… the wrapper script

Signed-off-by: Krisztián Varga <krisztian.varga@tngtech.com>
@vktng vktng force-pushed the improve_integ_test_wrapper_script branch from c7c861a to fee0c71 Compare August 24, 2022 08:56
@LKreutzer LKreutzer enabled auto-merge (squash) August 24, 2022 09:37
@LKreutzer LKreutzer merged commit b45c364 into magma:master Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel changes for the Bazelification effort component: agw Access gateway-related issue size/L Denotes a Pull Request that changes 100-499 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add improvements to the bazel/scripts/run_integ_tests.sh script
4 participants