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

fix(agw): fix incorrect asserts in agw integ tests #14012

Merged
merged 3 commits into from
Sep 28, 2022

Conversation

wolfseb
Copy link
Contributor

@wolfseb wolfseb commented Sep 26, 2022

Signed-off-by: Sebastian Wolf sebastian.wolf@tngtech.com

Summary

  • Refactor incorrect usage of assertTrue to assert ... == ... for response message asserts
  • Change all assertEqual in the touched tests to the new syntax as well

Test Plan

Run the touched integ tests

Additional Information

  • This change is backwards-breaking

@wolfseb wolfseb requested review from a team and rsarwad September 26, 2022 07:54
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines. label Sep 26, 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

@github-actions github-actions bot added the component: agw Access gateway-related issue label Sep 26, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2022

feg-workflow

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

Results for commit edb74fd.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2022

dp-workflow

14 tests   14 ✔️  2m 15s ⏱️
  1 suites    0 💤
  1 files      0

Results for commit edb74fd.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2022

agw-workflow

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

Results for commit edb74fd.

♻️ This comment has been updated with latest results.

response.msg_type,
s1ap_types.tfwCmd.UE_CTX_REL_IND.value,
)
assert response.msg_type == s1ap_types.tfwCmd.UE_CTX_REL_IND.value
Copy link
Contributor

Choose a reason for hiding this comment

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

The similar changes are required in other test cases, test_attach_detach_secondary_pdn_no_disconnect.py, test_attach_detach_multiple_secondary_pdn.py, test_attach_implicit_detach_timer_expiry.py, test_enb_partial_reset_multi_ue.py, test_enb_partial_reset_with_unknown_ue_s1ap_ids.py, test_secondary_pdn_reject_multiple_sessions_not_allowed_per_apn.py and many more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I am aware that about 100 tests or so still use self.assertEqual and similar functions instead of assert. The goal of this PR was to just fix the incorret assertTrue, where an assertEqual should have been used. A switch from the old to the new syntax in all tests will be done in a separate PR (maybe these changes here should be moved to that PR as well then?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nvm, I added these changes and separated them into several commits by fixing the incorrect asserts and changing the syntax

Signed-off-by: Sebastian Wolf <sebastian.wolf@tngtech.com>
This commit changes
* self.assertEqual and self.assertListEqual to assert ... == ...
* self.assertTrue to assert
* self.assertGreater to assert ... > ...
* self.assertGreaterEqual to assert ... >= ...
* self.assertLessEqual to assert ... <= ...
* self.assertIn to assert ... in ...
* self.assertIsNotNone to assert ... is not None

Signed-off-by: Sebastian Wolf <sebastian.wolf@tngtech.com>
@wolfseb wolfseb force-pushed the fix_wrong_asserts_in_s1ap_tests branch from d567e31 to eb8a8cf Compare September 28, 2022 00:46
@pull-request-size pull-request-size bot added size/XXL Denotes a Pull Request that changes 1000+ lines. and removed size/M Denotes a PR that changes 30-99 lines. labels Sep 28, 2022
Signed-off-by: Sebastian Wolf <sebastian.wolf@tngtech.com>
@wolfseb wolfseb force-pushed the fix_wrong_asserts_in_s1ap_tests branch from eb8a8cf to edb74fd Compare September 28, 2022 01:05
Copy link
Contributor

@rsarwad rsarwad left a comment

Choose a reason for hiding this comment

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

LGTM

@voisey voisey merged commit 408bfc9 into magma:master Sep 28, 2022
pruthvihebbani pushed a commit to pruthvihebbani/magma that referenced this pull request Oct 10, 2022
* fix(agw): fix incorrect usage of self.assertTrue

Signed-off-by: Sebastian Wolf <sebastian.wolf@tngtech.com>

* chore(agw): change to new pytest assert syntax

This commit changes
* self.assertEqual and self.assertListEqual to assert ... == ...
* self.assertTrue to assert
* self.assertGreater to assert ... > ...
* self.assertGreaterEqual to assert ... >= ...
* self.assertLessEqual to assert ... <= ...
* self.assertIn to assert ... in ...
* self.assertIsNotNone to assert ... is not None

Signed-off-by: Sebastian Wolf <sebastian.wolf@tngtech.com>

* chore(agw): cleanup line lengths from previous commits

Signed-off-by: Sebastian Wolf <sebastian.wolf@tngtech.com>

Signed-off-by: Sebastian Wolf <sebastian.wolf@tngtech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: agw Access gateway-related issue size/XXL Denotes a Pull Request that changes 1000+ lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants