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.

Sign up for GitHub

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

Jump to bottom

DEV-45405 - Fix grafana tests #29

Merged
merged 6 commits into from
Oct 6, 2024
Merged

DEV-45405 - Fix grafana tests #29

merged 6 commits into from
Oct 6, 2024

Conversation

yasmin-tr
Copy link

@yasmin-tr yasmin-tr commented Jun 17, 2024 •
edited
Loading

fix the failing grafana unit tests so we can have green pipeline

tests fixed:

  • api_notification_channel_test.go - fixed to expected strings returned from notifications
  • error_handling_test.go - fixed expected response due to our change in errors
  • api_plugins_test.go - updated the expectedListResp.json
  • dashboard_test.go - TestDashboardVersionsAPIEndpoint
  • templates_test.go - removed orgId from urls

also:

  • changed implementation in alertmanager.go NewAlertmanager() to avoid null exception if there is no ParsedAppUrl provided in config (fixes a lot of tests)
  • changed implementation in navtree.go to remove the connections section (datasources) in another place, to help tests pass and makes more sense.

@yasmin-tr
DEV-45405 - Fix grafana unit tests
a474edb 
api_notification_channel_test.go
error_handling_test.go
api_plugins_test.go - updated the expectedListResp.json
@yasmin-tr yasmin-tr changed the base branch from main to v10.4.x-logzio June 17, 2024 08:07
@yasmin-tr
Copy link
Author

run-test

1 similar comment
@npololnskii
Copy link

run-test

@yasmin-tr
Copy link
Author

run-test

@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions!

@github-actions github-actions bot added the stale label Aug 11, 2024
@yasmin-tr
Copy link
Author

run-test

@yasmin-tr yasmin-tr removed the stale label Aug 11, 2024
@npololnskii
Copy link

run-test

2 similar comments
@npololnskii
Copy link

run-test

@npololnskii
Copy link

run-test

@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions!

@github-actions github-actions bot added the stale label Sep 21, 2024
@yasmin-tr yasmin-tr changed the title DEV-45405 - Fix grafana unit tests DEV-45405 - Fix grafana tests Sep 22, 2024
@yasmin-tr yasmin-tr removed the stale label Sep 22, 2024
@yasmin-tr
Copy link
Author

run-test

@yasmin-tr
* fix templates_test.go - removed orgId from urls
61ff1e3 
* changed implementation in alertmanager.go NewAlertmanager() to avoid null exception if there is no ParsedAppUrl provided in config (fixes a lot of tests)
* changed implementation in navtree.go to remove the connections section (datasources) in another place, to help tests pass and makes more sense.
@yasmin-tr
Copy link
Author

run-test

copyhold
copyhold approved these changes Sep 25, 2024
View reviewed changes
Copy link

@copyhold copyhold left a comment

Choose a reason for hiding this comment

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

i hope you checked that datasource menu is really not visible...

Jonathan-Eng
Jonathan-Eng approved these changes Oct 1, 2024
View reviewed changes
pkg/services/navtree/navtreeimpl/navtree.go
@@ -485,7 +487,7 @@ func (s *ServiceImpl) buildDataConnectionsNavLink(c *contextmodel.ReqContext) *n

baseUrl := s.cfg.AppSubURL + "/connections"

if false && hasAccess(datasources.ConfigurationPageAccess) { // LOGZ.IO GRAFANA CHANGE :: hide datasources menu, should be done by altering permissions
if hasAccess(datasources.ConfigurationPageAccess) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this has no Grafana update msg?

Copy link
Author

@yasmin-tr yasmin-tr Oct 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this is a revert to original code, that's why i removed the logz comment (i simply moved the logz change to the part that calls this function to make the tests pass 🙏 )

@yasmin-tr
Copy link
Author

run-test

@yasmin-tr yasmin-tr merged commit ada530d into v10.4.x-logzio Oct 6, 2024
11 of 16 checks passed
@yasmin-tr yasmin-tr deleted the DEV-45405-fix-grafana-unit-tests branch November 4, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@copyhold copyhold copyhold approved these changes

@Jonathan-Eng Jonathan-Eng Jonathan-Eng approved these changes

@ohadza ohadza Awaiting requested review from ohadza

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yasmin-tr @npololnskii @copyhold @Jonathan-Eng