Skip to content

Conversation

@AAYUSH2091
Copy link
Contributor

@AAYUSH2091 AAYUSH2091 commented Oct 15, 2025

SUMMARY

Fixed multiple integration test failures in cisco.iosxr collection to ensure code changes don't break device functionality.

Ruchip16

This comment was marked as resolved.

Copy link
Contributor

@KB-perByte KB-perByte left a comment

Choose a reason for hiding this comment

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

I am fine with the test fixes just some comments for clarification.

from lxml import etree

HAS_XML = True
HAS_LXML = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change needed?
And this doesnot look like just test fixes.

Do we have a issue or ticket tracking this change?
Also, we would need a changelog if this change is relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so running all integration tests locally i have faced SSH connection issues, especially with these modules:
iosxr_banner
iosxr_system
iosxr_user
the existing error management complicates the process of identifying whether these are -
Authentic SSHor any network problems
Missing or incompatible library dependencies like ncclient or lxml
so what this change achieve like it helps collecting complete traceback of ncclient and lxml import errors
including xml.etree.Element as a backup when lxml encounter problems
and also employing missing_required_lib to get uniform error messages

module.fail_json(msg="ncclient is not installed")
if not HAS_XML:
module.fail_json(msg="lxml is not installed")
module.fail_json(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is good.

HAS_NCCLIENT = True
except ImportError:
HAS_NCCLIENT = False
NCCLIENT_IMP_ERR = traceback.format_exc()
Copy link
Contributor

Choose a reason for hiding this comment

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

this change is expected?

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