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 lowercase capture groups #1983

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mjbear
Copy link
Contributor

@mjbear mjbear commented Jan 19, 2025

  • Fix lowercase capture groups (should be uppercase per project guidelines)
  • In these same (lowercase cap grp) templates only, switch literal whitespace to whitespace regexes

@mjbear mjbear marked this pull request as ready for review January 19, 2025 23:57
@evilmonkey19
Copy link
Contributor

evilmonkey19 commented Jan 28, 2025

I suggest adding a test in the templates that check that all the values are in uppercase. It shouldn't be too difficult. Just checking the value will force that the TextFSM templates fails.

@mjbear
Copy link
Contributor Author

mjbear commented Jan 30, 2025

I suggest adding a test in the templates that check that all the values are in uppercase. It shouldn't be too difficult. Just checking the value will force that the TextFSM templates fails.

Good idea.
I'm pulling this to a Draft while I work on test cases. 😀

@mjbear mjbear marked this pull request as draft January 30, 2025 13:09
@evilmonkey19
Copy link
Contributor

evilmonkey19 commented Jan 30, 2025 via email

@mjbear
Copy link
Contributor Author

mjbear commented Jan 31, 2025

Whenever i have some i can add the tests cases, it should extremely easy :)

😅
I appreciate the nudge, haha.
I'll own these "lowercase" test cases for the initial development and take your code review suggestions. Sound good? 😁

@evilmonkey19
Copy link
Contributor

evilmonkey19 commented Jan 31, 2025 via email

@mjbear
Copy link
Contributor Author

mjbear commented Jan 31, 2025

Sure! Sorry, i was just trying to help :). I can do then some code review 😜

@evilmonkey19
All good!
I really wanted to work on this piece. 🫣
With everything going on in work/life, this motivated me to follow through to create these unit tests.

I made it happen today ... so you'll find a new test case. 🎉 😀

@evilmonkey19
Copy link
Contributor

It looks awesome! I like your trick of the rf I didn't know you could do that 📓. I learn something new today 😄

@mjbear
Copy link
Contributor Author

mjbear commented Feb 1, 2025

It looks awesome! I like your trick of the rf I didn't know you could do that 📓. I learn something new today 😄

😎👍
I've decided I may as well compile the regex as well. It may not result in a performance gain, but I can't really see a reason not to.

@mjbear mjbear marked this pull request as ready for review February 5, 2025 12:14
@jvanderaa
Copy link
Contributor

@mjbear I absolutely LOVE this and the test set up. I just want to verify, that when the test fails, the system continues to test until all of the files have been looked at right?

@mjbear
Copy link
Contributor Author

mjbear commented Feb 14, 2025

@mjbear I absolutely LOVE this and the test set up. I just want to verify, that when the test fails, the system continues to test until all of the files have been looked at right?

Yes - it continues on with the parsed data checks as well.

I began using this and it failed on IKEv1 and IPv6 (plus a couple of other items) initially until I converted them to uppercase.

Example: https://github.com/networktocode/tree/master/ntc_templates/templates/cisco_asa_show_running-config_all_crypto_map.textfsm
Commit: e60ab59

I checked the GH Actions and apparently I caught the lowercase ones before I pushed the commit to GitHub.

I checked out commit c4e61ad, pulled a copy of the new Python test from commit 324fd65, and re-ran tests to provide output. Note the failed and passed numbers in the summary. 🙂

🎯 Edit: @jvanderaa This is to say that IKEv1 does not pass the new lowercase test case, but continues parsing.

============================================================== short test summary info ===============================================================
FAILED tests/test_capture_group_case.py::test_uppercase_capture_group[./ntc_templates/templates/oneaccess_oneos_show_voice_voice-port_pri_all.textfsm] - AssertionError: assert False
FAILED tests/test_capture_group_case.py::test_uppercase_capture_group[./ntc_templates/templates/cisco_asa_show_running-config_all_crypto_map.textfsm] - AssertionError: assert False
FAILED tests/test_capture_group_case.py::test_uppercase_capture_group[./ntc_templates/templates/cisco_xr_show_lldp_neighbors_detail.textfsm] - AssertionError: assert False
FAILED tests/test_capture_group_case.py::test_uppercase_capture_group[./ntc_templates/templates/cisco_xr_show_processes_cpu.textfsm] - AssertionError: assert False
=============================================== 4 failed, 3324 passed, 18 warnings in 91.60s (0:01:31) ===============================================

I didn't change any capture group names (just case) so this wouldn't be a breaking change, would it?
(Or is it the test case addition?)
My mind wants to know and grow, hehe. 😁 Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants