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

[Marketplace Contribution] Content Testing - Content Pack Update #28959

Conversation

xsoar-bot
Copy link
Contributor

Status

  • In Progress
  • Ready
  • In Hold - (Reason for hold)

Contributor

@rurhrlaub

Notes

UnitTestLayout picked up script ID GUIDs in some cases and several scripts also picked up GUIDs. All replaced by name references:

UnitTestSetField
UnitTestLoadFieldsList
UnitTestLoadContextList
UnitTestSaveContextList
UnitTestSaveFieldsList
UnitTestLoadContext
UnitTestLoadFields
UnitTestCoverage
UnitTest
UnitTestPlaybookAnalyzer
ContentDependencies
UpgradeCheck
ChangeHistory

Video Link

Short demo video of the Pack usage. Speeds up the review. Optional but recommended. Use a video sharing service such as Google Drive or YouTube.

@content-bot content-bot added Contribution Thank you! Contributions are always welcome! External PR Community Support Level Indicates that the contribution is for Community supported pack labels Aug 14, 2023
@content-bot content-bot changed the base branch from master to contrib/xsoar-contrib_rurhrlaub-contrib-ContentTesting-7 August 14, 2023 15:15
@content-bot
Copy link
Collaborator

Thank you for your contribution. Your generosity and caring are unrivaled! Rest assured - our content wizard @RosenbergYehuda will very shortly look over your proposed changes.

Copy link
Contributor

@RosenbergYehuda RosenbergYehuda left a comment

Choose a reason for hiding this comment

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

Great job @rurhrlaub!
I added some small fixes, please take a look.
If you have any questions, feel free to response back on my comments.

Packs/ContentTesting/Scripts/UnitTestCase/UnitTestCase.py Outdated Show resolved Hide resolved
try:
args = demisto.args()
testName = args.get("testName", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove these 3 lines in to the 'try' block?
It does not need to be there beucus there is no scenario for it to fail, and the way it is now will not execute, because in the bottom the 'except Exception' is out of the 'try' block and we call 'testName', which is inside of the 'try' block.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. No lines removed and exception still valid?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, please ignore this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @rurhrlaub, how are you?
I've messaged you via Slack. Is "David Uhrlaub" your username there?
I'm currently looking into this unusual problem of lines moving without your touch, and would greatly value your responses.

@@ -1,5 +1,6 @@
import demistomock as demisto # noqa: F401
from CommonServerPython import * # noqa: F401
# Final Test: 6.10
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be removed.

Suggested change
# Final Test: 6.10

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@@ -1,6 +1,6 @@
import demistomock as demisto # noqa: F401
from CommonServerPython import * # noqa: F401

# Final Test: 6.10
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be removed, correct?

Suggested change
# Final Test: 6.10

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@@ -1,5 +1,6 @@
import demistomock as demisto # noqa: F401
from CommonServerPython import * # noqa: F401
# Final Test: 6.10
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@@ -1,5 +1,6 @@
import demistomock as demisto # noqa: F401
from CommonServerPython import * # noqa: F401
# Final Test: 6.10
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@@ -1,5 +1,6 @@
import demistomock as demisto # noqa: F401
from CommonServerPython import * # noqa: F401
# Final Test: 6.10
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@@ -1,5 +1,6 @@
import demistomock as demisto # noqa: F401
from CommonServerPython import * # noqa: F401
# Final Test: 6.10
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@melamedbn
Copy link
Contributor

Dear @rurhrlaub,

I hope this message finds you well. Firstly, I want to express my appreciation for your efforts in contributing to the content. Your dedication to the project is evident, and I wanted to share a couple of suggestions to further enhance the quality of the work:

Misconfigurations

  1. In the layout, there are buttons that have an ID instead of the script name which cause a failure. Please use the script name instead of the id.
    Can be found in the following buttons:
    Check Content Pack Upgrade Impacts on Custom Content (Content Assessment tab)
    Get Commit History for Local Changes (Content Assessment tab)
    All of the 'Content Testing' and 'Playbook Analysis' tab buttons.

    image

  2. The incident field has a change relating to the script name, please use the script name instead of the ID. (screenshot attached)

image

I trust that these suggestions will be valuable in fine-tuning the content. Once again, thank you for your commitment and hard work. I'm confident that with these minor tweaks, we'll be able to deliver an even more polished product to our users.

Warm regards,
Ben

@rurhrlaub
Copy link
Contributor

rurhrlaub commented Aug 22, 2023 via email

@melamedbn
Copy link
Contributor

melamedbn commented Aug 23, 2023

@rurhrlaub
Thank you for taking the time to review my feedback. I've updated the env and I see the script names are present.

Best regards,
Ben

@rurhrlaub
Copy link
Contributor

rurhrlaub commented Aug 23, 2023 via email

@rurhrlaub
Copy link
Contributor

rurhrlaub commented Aug 23, 2023 via email

@CLAassistant
Copy link

CLAassistant commented Aug 24, 2023

CLA assistant check
All committers have signed the CLA.

@RosenbergYehuda RosenbergYehuda merged commit 6cb2787 into demisto:contrib/xsoar-contrib_rurhrlaub-contrib-ContentTesting-7 Aug 29, 2023
17 of 20 checks passed
RosenbergYehuda added a commit that referenced this pull request Aug 30, 2023
)

* [Marketplace Contribution] Content Testing - Content Pack Update (#28959)

* "contribution update to pack "Content Testing""

* Update incidentfield-Content_Testing_Unit_Test_Playbooks.json

 Fixed   "script": "UnitTestMultiSelect",

* remove the wired changes in the pr

* help the build pass

* validate

* avoiding breaking changes

* docker image

* docker image

---------

Co-authored-by: David Uhrlaub <90627446+rurhrlaub@users.noreply.github.com>
Co-authored-by: Yehuda <yrosenberg@paloaltonetworks.com>
Co-authored-by: Yehuda Rosenberg <90599084+RosenbergYehuda@users.noreply.github.com>

* change default

* add default value for unrequited argument

* RN

* fix validate

---------

Co-authored-by: xsoar-bot <67315154+xsoar-bot@users.noreply.github.com>
Co-authored-by: David Uhrlaub <90627446+rurhrlaub@users.noreply.github.com>
Co-authored-by: Yehuda <yrosenberg@paloaltonetworks.com>
Co-authored-by: Yehuda Rosenberg <90599084+RosenbergYehuda@users.noreply.github.com>
xsoar-bot added a commit to xsoar-contrib/content that referenced this pull request Oct 5, 2023
…isto#29308)

* [Marketplace Contribution] Content Testing - Content Pack Update (demisto#28959)

* "contribution update to pack "Content Testing""

* Update incidentfield-Content_Testing_Unit_Test_Playbooks.json

 Fixed   "script": "UnitTestMultiSelect",

* remove the wired changes in the pr

* help the build pass

* validate

* avoiding breaking changes

* docker image

* docker image

---------

Co-authored-by: David Uhrlaub <90627446+rurhrlaub@users.noreply.github.com>
Co-authored-by: Yehuda <yrosenberg@paloaltonetworks.com>
Co-authored-by: Yehuda Rosenberg <90599084+RosenbergYehuda@users.noreply.github.com>

* change default

* add default value for unrequited argument

* RN

* fix validate

---------

Co-authored-by: xsoar-bot <67315154+xsoar-bot@users.noreply.github.com>
Co-authored-by: David Uhrlaub <90627446+rurhrlaub@users.noreply.github.com>
Co-authored-by: Yehuda <yrosenberg@paloaltonetworks.com>
Co-authored-by: Yehuda Rosenberg <90599084+RosenbergYehuda@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Support Level Indicates that the contribution is for Community supported pack Contribution Thank you! Contributions are always welcome! docs-approved External PR post-demo Security Review
Projects
None yet
6 participants