Skip to content

Conversation

@eradman
Copy link
Collaborator

@eradman eradman commented Dec 24, 2024

What type of PR is this?

  • Tests

Description

Other tests and local settings sometimes caused this test to fail.

How is this tested?

  • Unit tests (pytest, jest)

@codecov
Copy link

codecov bot commented Jan 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.03%. Comparing base (d03a2c4) to head (3711d87).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7260      +/-   ##
==========================================
+ Coverage   64.01%   64.03%   +0.01%     
==========================================
  Files         163      163              
  Lines       13410    13410              
  Branches     1905     1905              
==========================================
+ Hits         8585     8587       +2     
+ Misses       4490     4489       -1     
+ Partials      335      334       -1     

see 1 file with indirect coverage changes

@eradman eradman force-pushed the alert-tests branch 2 times, most recently from 038ca94 to 334b9ad Compare May 9, 2025 17:39
@eradman
Copy link
Collaborator Author

eradman commented Sep 10, 2025

@yoshiokatsuneo if you have time, can you review this change? TestAlertRenderTemplate often fails in my dev environment because of settings in .env

@yoshiokatsuneo
Copy link
Contributor

@eradman

Thank you for improving test environment.

I wonder whether it is good idea to change/reset settings on every tests, as it may make test code a bit messy...
How about using separate .env file for testing ( like .env_test ) ?

@yoshiokatsuneo
Copy link
Contributor

Or, possibly, we can statically setup settings on the base TestCase class ?

@eradman
Copy link
Collaborator Author

eradman commented Sep 10, 2025

How about using separate .env file for testing ( like .env_test ) ?

This is not a bad idea: there are probably other test cases that would fail with modified settings.

Remove hard-coded 'https://' when MULTI_ORG is enabled
@eradman eradman changed the title Use fixed environment for TestAlertRenderTemplate Multi-org: format base path, not including protocol Dec 17, 2025
@eradman
Copy link
Collaborator Author

eradman commented Dec 17, 2025

Instead of sanitizing the environment, I modified the utils.base_url() to prevent it from returning a hard-coded protocol (https://) when MULTI_ORG is enabled. As near as I can tell this is a bug, and not noticed by anyone.

Copy link
Contributor

@yoshiokatsuneo yoshiokatsuneo left a comment

Choose a reason for hiding this comment

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

@eradman

Thank you ! Actually, it looks a bug, and you fixed it !

@eradman eradman merged commit 262d46f into getredash:master Dec 18, 2025
15 of 16 checks passed
@eradman eradman deleted the alert-tests branch December 18, 2025 00:34
@eradman
Copy link
Collaborator Author

eradman commented Dec 18, 2025

Thanks for the review again @yoshiokatsuneo!
MULTI_ORG has been an experimental feature, so most users are not likely to hit this code path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants