[GitHub] [superset] betodealmeida commented on a change in pull request #13282: chore: migrate fixtures for core_tests.py

[GitBox <gi...@apache.org>]

betodealmeida commented on a change in pull request #13282:
URL: https://github.com/apache/superset/pull/13282#discussion_r580648966



##########
File path: tests/fixtures/core.py
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import json
+
+
+def bad_database_test_conn_request():
+    return json.dumps(
+        {"uri": "mssql+pymssql://url", "name": "examples", "impersonate_user": False,}
+    )
+
+
+def create_query_editor_request():

Review comment:
       Same here, I think it would be better to call this `query_editor_request` and use the fixture decorator (with `yield` instead of `return`, as it recommended).

##########
File path: tests/fixtures/core.py
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import json
+
+
+def bad_database_test_conn_request():

Review comment:
       IMHO this fixture is very restricted and we won't have the chance to reuse it in many places. I think it would be better to define this as a generic request:
   
   ```python
   def test_connection_request():
       return {
          "uri": database.safe_sqlalchemy_uri(),
          "name": "examples",
          "impersonate_user": False,
       }
   ```
   
   Then in our tests we should be able to do:
   
   ```python
   def test_testconn_failed_conn(self, username="admin"):
       connection = test_connection_request()
       connection["uri"] = "mssql+pymssql://url"
       form_data = json.dumps(connection)
       ...
   ```
   
   Note that there's a cooler way of using fixtures, though! Replace your function with one decorated with `@pytest.fixture` that [yields the result](https://docs.pytest.org/en/stable/fixture.html#yield-fixtures-recommended) instead, and [place it in `conftest.py`](https://docs.pytest.org/en/stable/fixture.html#conftest-py-sharing-fixtures-across-multiple-files):
   
   ```python
   @pytest.fixture
   def test_connection_request():
       yield {
          "uri": database.safe_sqlalchemy_uri(),
          "name": "examples",
          "impersonate_user": False,
       }
   ```
   
   When you use the `pytest.fixture` decorator you can write tests like this (it **doesn't work** with SupersetTestCase, though, has to be a function):
   
   ```python
   def test_testconn_failed_conn(test_connection_request):
       test_connection_request["uri"] = "mssql+pymssql://url"
       form_data = json.dumps(test_connection_request)
       ...
   ```
   
   Ie, `pytest` will automatically pass the fixture when you declare it in the function arguments (which is why fixtures should have memorable names as well). And note that if you place them inside `conftest.py` (either in the `tests` root or a subdirectory) they will be automatically applied without having to be imported.

##########
File path: tests/fixtures/core.py
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import json
+
+
+def bad_database_test_conn_request():
+    return json.dumps(
+        {"uri": "mssql+pymssql://url", "name": "examples", "impersonate_user": False,}
+    )
+
+
+def create_query_editor_request():
+    return {
+        "queryEditor": json.dumps(
+            {
+                "title": "Untitled Query 1",
+                "dbId": 1,
+                "schema": None,
+                "autorun": False,
+                "sql": "SELECT ...",
+                "queryLimit": 1000,
+            }
+        )
+    }
+
+
+def dist_bar_form_data(table_id):

Review comment:
       In pytest fixtures don't have an easy way of accepting parameters. You can either modify the response from the fixture like I suggested for the bad connection, or you can [parametrize it](https://docs.pytest.org/en/latest/example/parametrize.html#indirect-parametrization):
   
   ```python
   @pytest.fixture
   def dist_bar_form_data(request):
       table_id = request.param
       return { .... }  # note the use of return here
   ```
   
   Then:
   
   ```python
   @pytest.mark.parametrize('dist_bar_form_data', [[tbl_id]], indirect=True)
   def test_create_query_editor(dist_bar_form_data):
       ...
   ```
   
   Maybe there's a better way?




----------------------------------------------------------------
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.

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