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

chore: new alert rule documentation redirection tests #6822

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ahmadshaheer
Copy link
Collaborator

@ahmadshaheer ahmadshaheer commented Jan 15, 2025

Summary

  • Check the redirection to documentation for each alert type from alert type selection page
  • Check the redirection to documentation for each alert type from alert creation page

Related Issues / PR's

close https://github.com/SigNoz/engineering-pod/issues/1863
close https://github.com/SigNoz/engineering-pod/issues/1862

Screenshots

NA

Affected Areas and Manually Tested Areas


Important

Add tests for alert rule documentation redirection and refactor AnomalyAlertEvaluationView.tsx to use destructured Search from Input.

  • Tests:
    • Add AlertRuleDocumentationRedirection.test.tsx to test redirection to documentation for each alert type from alert type selection and alert creation pages.
    • Mock react-router-dom and uplot in tests to simulate environment.
  • Constants:
    • Add AlertTypesWithoutAnomaly, ALERT_TYPE_URL_MAP, and ALERT_TYPE_TO_TITLE in constants.ts for managing alert types and their documentation URLs.
  • Refactor:
    • Change import of Search to destructure from Input in AnomalyAlertEvaluationView.tsx.

This description was created by Ellipsis for e4a1a0b. It will automatically update as commits are pushed.

@ahmadshaheer ahmadshaheer requested a review from YounixM as a code owner January 15, 2025 06:04
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@github-actions github-actions bot added the chore label Jan 15, 2025
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 0a7afe1 in 1 minute and 20 seconds

More details
  • Looked at 190 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/container/CreateAlertRule/AlertRuleDocumentationRedirection.test.tsx:94
  • Draft comment:
    Consider defining the text 'Click here to see how to create a sample alert.' as a constant to avoid repetition and potential errors if the text needs to be updated.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test file uses hardcoded text 'Click here to see how to create a sample alert.' which is repeated multiple times. It would be better to define this text as a constant to avoid repetition and potential errors if the text needs to be updated.
2. frontend/src/container/CreateAlertRule/AlertRuleDocumentationRedirection.test.tsx:131
  • Draft comment:
    Consider defining the text 'Check an example alert' as a constant to avoid repetition and potential errors if the text needs to be updated.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test file uses hardcoded text 'Check an example alert' which is repeated multiple times. It would be better to define this text as a constant to avoid repetition and potential errors if the text needs to be updated.
3. frontend/src/container/CreateAlertRule/constants.ts:9
  • Draft comment:
    Use predefined constants for URLs instead of hardcoding them to maintain consistency.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_nUXejlBYg6wNctPO


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on bc6da71 in 34 seconds

More details
  • Looked at 158 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/container/CreateAlertRule/constants.ts:1
  • Draft comment:
    The import for AlertTypes is unused and can be removed to clean up the code.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The import statement for AlertTypes is not used in the file. It should be removed to clean up the code.
2. frontend/src/container/CreateAlertRule/constants.ts:42
  • Draft comment:
    The ALERT_TYPE_TO_TITLE map should use AlertTypesWithoutAnomaly instead of AlertTypes for consistency.
	[AlertTypesWithoutAnomaly.METRICS_BASED_ALERT]: 'metric_based_alert',
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. frontend/src/container/AnomalyAlertEvaluationView/AnomalyAlertEvaluationView.tsx:4
  • Draft comment:
    Avoid using the component/index.tsx file structure approach. The change to destructure Search from Input is a good practice.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statement for Search from antd was changed to use destructuring from Input. This is a minor change but aligns with the rule of avoiding the component/index.tsx file structure approach.

Workflow ID: wflow_XTeiXlaUtAm8JGKH


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@ahmadshaheer ahmadshaheer force-pushed the chore/alert-documentation-redirect-tests branch from bc6da71 to e4a1a0b Compare January 15, 2025 09:42
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on e4a1a0b in 33 seconds

More details
  • Looked at 158 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/container/CreateAlertRule/constants.ts:42
  • Draft comment:
    The ALERT_TYPE_TO_TITLE map should use AlertTypesWithoutAnomaly for consistency.
	[AlertTypesWithoutAnomaly.METRICS_BASED_ALERT]: 'metric_based_alert',
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. frontend/src/container/AnomalyAlertEvaluationView/AnomalyAlertEvaluationView.tsx:4
  • Draft comment:
    Remove the unnecessary import of Search from antd as it is already destructured from Input.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statement for Search from antd is not necessary since Input is already imported and destructured to get Search. This is a minor issue but should be addressed for code cleanliness.

Workflow ID: wflow_A9JTPkVJeYvDhxhe


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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

Successfully merging this pull request may close these issues.

2 participants