[GitHub] [superset] john-bodley commented on a diff in pull request #24671: fix: cache warmup for non-legacy charts

["john-bodley (via GitHub)" <gi...@apache.org>]

john-bodley commented on code in PR #24671:
URL: https://github.com/apache/superset/pull/24671#discussion_r1260624573


##########
superset/charts/commands/warm_up_cache.py:
##########
@@ -43,31 +45,51 @@ def __init__(
     def run(self) -> dict[str, Any]:
         self.validate()
         chart: Slice = self._chart_or_id  # type: ignore
+
         try:
             form_data = get_form_data(chart.id, use_slice_data=True)[0]
-            if self._dashboard_id:

Review Comment:
   This logic (for the most part) has just been indented.



##########
superset/views/core.py:
##########
@@ -825,43 +827,15 @@ def warm_up_cache(  # pylint: disable=too-many-locals,no-self-use
                 .all()
             )
 
-        result = []

Review Comment:
   Adhering to the DRY principle.



##########
tests/integration_tests/core_tests.py:
##########
@@ -14,72 +14,67 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
-# isort:skip_file

Review Comment:
   I have no idea why this was here.



##########
tests/integration_tests/charts/api_tests.py:
##########
@@ -1710,12 +1714,16 @@ def test_gets_owned_created_favorited_by_me_filter(self):
         assert data["result"][0]["slice_name"] == "name0"
         assert data["result"][0]["datasource_id"] == 1
 
-    @pytest.mark.usefixtures(
-        "load_energy_table_with_slice", "load_birth_names_dashboard_with_slices"
+    @parameterized.expand(

Review Comment:
   Ensure we can successfully warm up legacy and non-legacy charts.



##########
tests/integration_tests/charts/api_tests.py:
##########
@@ -14,37 +14,41 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
-# isort:skip_file

Review Comment:
   I have no idea why this was here.



##########
tests/integration_tests/core_tests.py:
##########
@@ -386,9 +381,7 @@ def test_databaseview_edit(self, username="admin"):
         database.allow_run_async = False
         db.session.commit()
 
-    @pytest.mark.usefixtures(
-        "load_energy_table_with_slice", "load_birth_names_dashboard_with_slices"

Review Comment:
   The `load_energy_table_with_slice` fixture is not required.



##########
tests/integration_tests/charts/api_tests.py:
##########
@@ -1710,12 +1714,16 @@ def test_gets_owned_created_favorited_by_me_filter(self):
         assert data["result"][0]["slice_name"] == "name0"
         assert data["result"][0]["datasource_id"] == 1
 
-    @pytest.mark.usefixtures(
-        "load_energy_table_with_slice", "load_birth_names_dashboard_with_slices"

Review Comment:
   The `load_energy_table_with_slice` fixture is not required.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org