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: new integ tests are bazelified #13623

Merged
merged 3 commits into from
Aug 22, 2022

Conversation

nstng
Copy link
Contributor

@nstng nstng commented Aug 16, 2022

Summary

Main target here was to do #13588. But it came up that one python file - gpp_types.py - was not yet bazelified (this is used in integ tests, but somehow bazel has access on magma_test) and that there are three new integ tests that need to be bazelified.

Note that two of the integ tests seem to be setup and tear down "tests" for not yet activated ipv6 tests. But imho it is a good idea to bazelify them for now so that this logic is tested as well. The basilification of those tests was removed after a review discussion. This will be re-added when respective tests for these setup and teardown "tests" are added. Or they are included in the respective tests directly.

Test Plan

new tests

  • ./bazel/scripts/run_integ_tests.sh lte/gateway/python/integ_tests/s1aptests:test_attach_detach_flaky_retry_success

gpp_types

On a vm != magma_test check if bazel run produces ModuleNotFound exceptions (the tests will fail! but the ModuleNotFound exception should come before the test failure). E.g., use

#!/usr/bin/env bash

LOGGING_FILE="/tmp/test_import_logging.log"

A=$(bazel query "kind(py_test, //lte/gateway/python/integ_tests/s1aptests/...)")
for test in $A
do
  echo "CHECKING ${test}"
  bazel run "${test}" --define=on_magma_test=1 2>&1  | tee "${LOGGING_FILE}"
  RESULT=$(grep -m 1 "ModuleNotFoundError" "${LOGGING_FILE}" || [[ $? == 1 ]])
  if [[ "${RESULT}" != "" ]];
  then
    exit 1
  fi
done

updated py files check

In bazel/scripts/check_py_bazel.sh remove line 56-81and check that the output corresponds to the removed lines. Note that some files are covered by excluding complete directories.

Additional Information

  • This change is backwards-breaking

@nstng nstng requested review from a team and electronjoe August 16, 2022 10:16
@pull-request-size pull-request-size bot added the size/L Denotes a Pull Request that changes 100-499 lines. label Aug 16, 2022
@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

@nstng nstng requested review from vktng and removed request for electronjoe August 16, 2022 10:16
@github-actions github-actions bot added the component: agw Access gateway-related issue label Aug 16, 2022
@nstng nstng linked an issue Aug 16, 2022 that may be closed by this pull request
@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 1b65ccc.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 16, 2022

dp-workflow

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

Results for commit 1b65ccc.

♻️ 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 ✔️  3m 51s ⏱️
    2 suites      4 💤
    2 files        0

Results for commit 1b65ccc.

♻️ This comment has been updated with latest results.

@nstng nstng requested a review from LKreutzer August 22, 2022 07:32
bazel/scripts/check_py_bazel.sh Outdated Show resolved Hide resolved
size = "small",
srcs = ["test_enable_ipv6_iface.py"],
imports = [LTE_ROOT],
tags = TAG_PRECOMMIT_TEST,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add new tags to the file bazel/test_constants.bzl:

TAG_IPV6_TEST_ENABLE = ["ipv6_enable"] + TAG_MANUAL
TAG_IPV6_TEST_DISABLE = ["ipv6_disable"] + TAG_MANUAL

And tag the tests with these labels.
Even if it does not seem to break something, we should not run them out of order with the other precommit tests.

Copy link
Contributor Author

@nstng nstng Aug 22, 2022

Choose a reason for hiding this comment

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

discussed offline - remove from BUILD.bazel and add to check again.

  • these "tests" are for setup and do not test something currently
  • we will ask the owner if this can be included in specific setup/teardown steps in tests (the two steps seem to be sufficiently fast)

@nstng nstng requested a review from vktng August 22, 2022 13:25
@LKreutzer LKreutzer added the bazel changes for the Bazelification effort label Aug 22, 2022
Copy link
Contributor

@vktng vktng left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>
Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>
Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>
@nstng nstng merged commit f957bc1 into magma:master Aug 22, 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.

Remove S1AP tests from python script list for bazelification check
3 participants