Skip to content
This repository has been archived by the owner on Jan 10, 2025. It is now read-only.

feat: support xml files #9

Merged
merged 7 commits into from
Mar 14, 2022
Merged

feat: support xml files #9

merged 7 commits into from
Mar 14, 2022

Conversation

nandinivij
Copy link
Collaborator

@nandinivij nandinivij commented Feb 24, 2022

  • check added to xml_format_checker.py to fail the test if we have a mixture of .log and. XML files.

  • XML format check is the first test. If this fails none of the other tests will run.

  • All other tests are moved to support both XML and .log files.

Required to support - splunk/pytest-splunk-addon#550 with unit tests
Please review.
tested Github action with juniper addon - https://github.com/splunk/splunk-add-on-for-juniper/pull/277

@nandinivij nandinivij marked this pull request as ready for review February 24, 2022 23:37
@nandinivij nandinivij requested review from a user and hsekowski-splunk February 24, 2022 23:37
@nandinivij nandinivij marked this pull request as draft February 28, 2022 17:14
@nandinivij nandinivij marked this pull request as ready for review February 28, 2022 17:23
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Can we have a function to return a list of files that we want to check and reuse it everywhere?
In case we want to change something in the future, we will need to apply the change only once.

else:
logger.debug(fname + ":Invalid input file format")
test_status = False
if contains_XML and contains_log:

Choose a reason for hiding this comment

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

this condition will be always logically False

Copy link
Collaborator Author

@nandinivij nandinivij Feb 28, 2022

Choose a reason for hiding this comment

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

This condition will be true if we have a mixture of.xml and .log files in the requirement test folder. This is not allowed. It was a check requested to be added to avoid confusion

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Screen Shot 2022-03-01 at 1 56 21 PM
If combination of files we want to add failure case

Choose a reason for hiding this comment

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

@nandinivij ,
Take a look at splunk/pytest-splunk-addon#549 please

Regarding this point
I don't want this PR cause need of braking change.
I want ppl to start using .xml first and work on tool that helps to migrate from .log to .xml
The tool is not a mandatory if we want to change file extension only, but there are much more things that needs to be done to PSA samples in the future - like merging knowledge and requirement tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi, @hsekowski-splunk I am aware of the future scope regarding KO and req test samples, But this PR is to support the .xml or .log files in unit tests. As we already merged the PR to support both in PSA.
Without this PR -
Req test - will support .xml or .log
But unit test will throw an error of invalid file format when it finds a .xml file.
https://github.com/splunk/splunk-add-on-for-juniper/runs/5395821561?check_suite_focus=true
Screen Shot 2022-03-02 at 9 46 18 AM

With this PR
Unit tests will also support both .xml /.log format.
Added check here - we restrict the combination of .log and .xml files in the requirement_test/logs folder. The reasoning behind restricting .log and .xml is when the combination is present it can create confusion if one means different from the other.

This is not a breaking change. I am testing this branch with Juniper addon existing logs. Please find test run here - https://github.com/splunk/splunk-add-on-for-juniper/runs/5396003674?check_suite_focus=true

Hope this explains this PR. Let me know if we need to hop on a quick chat

test_lib/xml_format_checker.py Outdated Show resolved Hide resolved
test_lib/xml_format_checker.py Outdated Show resolved Hide resolved
test_lib/xml_format_checker.py Outdated Show resolved Hide resolved
@nandinivij nandinivij marked this pull request as draft March 1, 2022 22:56
@nandinivij nandinivij marked this pull request as ready for review March 1, 2022 23:09
@nandinivij nandinivij requested review from hsekowski-splunk and a user March 1, 2022 23:09
else:
logger.debug(fname + ":Invalid input file format")
test_status = False
if contains_XML and contains_log:

Choose a reason for hiding this comment

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

@nandinivij ,
Take a look at splunk/pytest-splunk-addon#549 please

Regarding this point
I don't want this PR cause need of braking change.
I want ppl to start using .xml first and work on tool that helps to migrate from .log to .xml
The tool is not a mandatory if we want to change file extension only, but there are much more things that needs to be done to PSA samples in the future - like merging knowledge and requirement tests.

test_lib/test_cim.py Outdated Show resolved Hide resolved
@nandinivij nandinivij requested review from a user and hsekowski-splunk March 2, 2022 17:28
@nandinivij nandinivij merged commit 80cccd0 into main Mar 14, 2022
@nandinivij nandinivij deleted the feat/support_xml branch March 14, 2022 17:19
@srv-rr-github-token
Copy link

🎉 This PR is included in version 1.4.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants