Conversation
for more information, see https://pre-commit.ci
|
Thanks @bruno-portfolio! @schwehr can you take a look and see if this is what you had in mind? |
|
Thank you @bruno-portfolio! This is a great reference PR to start from. First, apologies that docs/contributing.md is so out of date. I'll try to cover the process to get tests into geemap here and then we'll try to get the contributing.md updated. Let's start with the simple and working test_cli.py. I verified it works for me locally: I've added comments you can use for that new PR. |
There was a problem hiding this comment.
Please copy this file to a fresh branch and make a new PR with just test_deck.py
|
Now that test_cli.py is in, how about test_deck.py? I confirmed it works locally for me: |
schwehr
left a comment
There was a problem hiding this comment.
Awesome to have test_deck.py in. How about test_plotlymap.py next?
| @@ -0,0 +1,262 @@ | |||
| """Tests for the plotlymap module.""" | |||
|
|
|||
| from __future__ import annotations | |||
| except ImportError: | ||
| PLOTLY_AVAILABLE = False | ||
|
|
||
| PLOTLYMAP_MODULE = None |
There was a problem hiding this comment.
Remove:
PLOTLYMAP_MODULE = None
IMPORT_ERROR = None
| IMPORT_ERROR = None | ||
|
|
||
|
|
||
| def get_plotlymap(): |
| class PlotlymapTestCase(unittest.TestCase): | ||
|
|
||
| @classmethod | ||
| def setUpClass(cls) -> None: |
There was a problem hiding this comment.
Remove setUpClass, tearDownClass, and setUp
|
|
||
|
|
||
| @unittest.skipUnless(PLOTLY_AVAILABLE, "plotly not available") | ||
| class PlotlymapTestCase(unittest.TestCase): |
There was a problem hiding this comment.
Rename PlotlymapTestCase --> PlotlymapTest
| def test_add_controls_string(self) -> None: | ||
| with mock.patch("geemap.coreutils.ee_initialize"): | ||
| m = self.plotlymap.Map(ee_initialize=False) | ||
| m.add_controls("drawline") |
There was a problem hiding this comment.
This and the tests below should be able to check that the action was taken.
| url="https://tile.example.com/{z}/{x}/{y}.png", | ||
| name="Test Layer", | ||
| attribution="Test", | ||
| ) |
There was a problem hiding this comment.
There needs to be an assert at the end that tests to see if the action worked.
| with mock.patch("geemap.coreutils.ee_initialize"): | ||
| m = self.plotlymap.Map(ee_initialize=False) | ||
| layer = go.Scattermapbox(lat=[37.8], lon=[-122.4], name="test") | ||
| m.add_layer(layer, name="Test Layer") |
| with mock.patch("geemap.coreutils.ee_initialize"): | ||
| m = self.plotlymap.Map(ee_initialize=False) | ||
| layers = m.get_layers() | ||
| self.assertIsInstance(layers, dict) |
There was a problem hiding this comment.
What is the expected contents of layers in the default setup? What can be asserted? Same for the follow up tests.
| "value": [1.0, 0.5], | ||
| } | ||
| ) | ||
| m.add_heatmap(df) |
There was a problem hiding this comment.
Assert something about the map that tests if the heatmap was added
schwehr
left a comment
There was a problem hiding this comment.
How about doing test_foliumap.py next.
Mostly the same comments before. A notable new thing is using with to create temps and have them automatically cleanup when the code exists the with scope.
| except ImportError: | ||
| FOLIUM_AVAILABLE = False | ||
|
|
||
| FOLIUMAP_MODULE = None |
There was a problem hiding this comment.
Remove these lines
FOLIUMAP_MODULE = None
IMPORT_ERROR = None
| IMPORT_ERROR = None | ||
|
|
||
|
|
||
| def get_foliumap(): |
There was a problem hiding this comment.
Remove this function and put the imports above in the try with import folium
| } | ||
| ], | ||
| } | ||
| temp_path = os.path.join(self.temp_dir, "test.geojson") |
There was a problem hiding this comment.
Just do with tempfile.TemporaryDirectory() as tmpdir: here like in `test_common.py. And remove the temp dir stuff in the class setup.
| def test_to_html_saves_file(self) -> None: | ||
| with mock.patch("geemap.coreutils.ee_initialize"): | ||
| m = self.foliumap.Map(ee_initialize=False) | ||
| temp_path = os.path.join(self.temp_dir, "test_map.html") |
There was a problem hiding this comment.
Again use with to create the temp dir just for this test.
| temp_path = os.path.join(self.temp_dir, "test_map.html") | ||
| m.to_html(temp_path) | ||
| self.assertTrue(os.path.exists(temp_path)) | ||
|
|
There was a problem hiding this comment.
Please assert something about the content for the file that is written.
| class FoliumapTestCase(unittest.TestCase): | ||
|
|
||
| @classmethod | ||
| def setUpClass(cls) -> None: |
There was a problem hiding this comment.
Remove all the setup and teardown methods
| with mock.patch("geemap.coreutils.ee_initialize"): | ||
| with mock.patch("geemap.common.is_arcpy", return_value=False): | ||
| m = self.foliumap.Map(ee_initialize=False) | ||
| self.assertEqual(m.setCenter, m.set_center) |
There was a problem hiding this comment.
I don't think we need to test this. It's just an alias.
| def test_zoom_to_bounds_valid_bounds(self) -> None: | ||
| with mock.patch("geemap.coreutils.ee_initialize"): | ||
| m = self.foliumap.Map(ee_initialize=False) | ||
| m.zoom_to_bounds([-122.5, 37.5, -122.0, 38.0]) |
There was a problem hiding this comment.
Assert something about the m instance that changed from the call.
| tiles="https://tile.example.com/{z}/{x}/{y}.png", | ||
| name="Test Layer", | ||
| attribution="Test", | ||
| ) |
There was a problem hiding this comment.
Assert something for the change in m. Same for all the other tests the do a change. They all have to assert something about the change.
| def test_setcontrolvisibility_alias(self) -> None: | ||
| with mock.patch("geemap.coreutils.ee_initialize"): | ||
| m = self.foliumap.Map(ee_initialize=False) | ||
| self.assertEqual(m.setControlVisibility, m.set_control_visibility) |
There was a problem hiding this comment.
Also remove this alias check
|
How about picking another test and doing the same style cleanup? Maybe test_osm.py? |
Summary
Adds comprehensive unit test coverage for multiple modules as requested in #2406.
Changes
Design Notes
Checklist