Convert tests from unittest to pytest format by RohanDisa · Pull Request #76 · xqlsystems/xarray-sql

RohanDisa · 2025-09-28T01:21:27Z

Convert df_test.py from unittest to pytest with fixtures and markers
Convert df_integrationtest.py to pytest format with integration markers
Update sql_test.py with pytest fixtures and modern assertions
Add pytest configuration to pyproject.toml
Format all test files with pyink
Add @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.gcs markers
Maintain existing project structure and Google-style docstrings

Tests now use modern pytest features while respecting project guidelines.

- Convert df_test.py from unittest to pytest with fixtures and markers - Convert df_integrationtest.py to pytest format with integration markers - Update sql_test.py with pytest fixtures and modern assertions - Add pytest configuration to pyproject.toml - Format all test files with pyink - Add @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.gcs markers - Maintain existing project structure and Google-style docstrings Tests now use modern pytest features while respecting project guidelines.

alxmrs

A few nits. I'd also like to confirm that all of the tests were ported. I think how I'm diffing things is a bit confusing to me; I just really want to be certain.

alxmrs · 2025-09-28T07:08:05Z

@@ -1,415 +0,0 @@
-<?xml version="1.0" standalone="no"?><!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd"><svg version="1.1" width="1200" height="2266" onload="init(evt)" viewBox="0 0 1200 2266" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:fg="http://github.com/jonhoo/inferno"><!--Flame graph stack visualization. See https://github.com/brendangregg/FlameGraph for latest version, and http://www.brendangregg.com/flamegraphs.html for examples.--><!--NOTES: --><defs><linearGradient id="background" y1="0" y2="1" x1="0" x2="0"><stop stop-color="#eeeeee" offset="5%"/><stop stop-color="#eeeeb0" offset="95%"/></linearGradient></defs><style type="text/css">


Thanks for deleting these; they've gone stale.

alxmrs · 2025-09-28T07:12:25Z

+# -------------------------
+class TestExplode:
+
+  @pytest.mark.unit


How do you intend to use this marker? What if we considered "unit" tests as the default type of test? Then, we could omit adding these markers to every test.

alxmrs · 2025-09-28T07:15:24Z

+    ds = next(iter(explode(air_dataset)))
    iselection = {dim: slice(0, s) for dim, s in ds.sizes.items()}
-    self.assertEqual(self.air.isel(iselection), ds)
+    assert air_dataset.isel(iselection).equals(ds)


Nice! This is slick.

alxmrs · 2025-09-28T07:26:04Z

+# -------------------------
+# Test Classes
+# -------------------------
+class TestExplode:


I am ok with omitting test classes! I'm happy to have everything just be functions, too. I leave the decision up to you.

alxmrs · 2025-09-28T10:46:12Z

-      LIMIT 30
-    """
-    ).to_pandas()
+  @pytest.fixture


Should we move this fixture to the top of the file?

alxmrs · 2025-09-28T10:47:05Z

 from .df import explode, read_xarray, block_slices, from_map, pivot, from_map_batched


+# -------------------------


(optional): I don't think we need these section comments, but I defer to your judgement if we should keep them (I do see some of their benefit).

alxmrs · 2025-09-28T10:50:13Z


 [tool.setuptools_scm]

+[tool.pytest.ini_options]


I think this config or the CI config needs to be updated in order to ensure tests run properly.

alxmrs · 2025-09-28T10:51:04Z

High level: Thank you for your contribution! This is such an improvement to how testing was done before :)

- Assumed tests to be default unit tests, so removed explicit markers. - Omitted classes for a cleaner, more readable structure. - Added all missing test cases that were previously skipped in df_test. - Moved all fixtures to the top for better organization. - Reverted to the original project.toml, which is sufficient for running tests.

RohanDisa · 2025-10-01T01:55:26Z

"Hi @alxmrs , I have made the requested changes. When you get a chance, could you please take another look? Thanks!

alxmrs

This is such an improvement! Thank you for your contribution.

I have one last high level piece of feedback. You're free to fix this and then merge, or merge and then follow up with it in a future PR/issue. It's not a deal breaker to me.

alxmrs · 2025-10-02T16:09:55Z

-class Era5TestCast(unittest.TestCase):
-
-  def test_open_era5(self):
+@pytest.mark.integration


One potential use for this marker would be to exclude integration tests from normal CI runs (and maybe run in a separate action called "integration" or "slow tests". The current way we handle this today is to use the file naming pattern "_integrationtest.py". I'm happy to do one of the following, but not both, of:

Moving this test to df_test.py and keeping the integration test marker

Removing the marker and keeping the file naming pattern to control what tests are run.

I leave it up to you to choose which of these patterns for managing slow tests to use. For now, I'm happy to have integration tests not be run in CI (since they aren't run there so far. That would be a good thing to do in the future; I'll file an issue about that soon).

(My apologies for not bringing this up earlier).

Hey @alxmrs, I have gone with option 1!

alxmrs · 2025-10-02T16:15:56Z

Before merging, please make sure all the checks pass.

Co-authored-by: Alex Merose <alex@openathena.ai> Signed-off-by: Rohan Disa <105740583+RohanDisa@users.noreply.github.com>

…e pytest markers.

alxmrs · 2025-10-03T10:23:08Z


+@pytest.mark.integration
+@pytest.mark.gcs
+def test_open_era5():


I like this option, but now, there needs to be something in the actions runner to exclude running this test, because it will take a long time (and may not even be working -- I think it requires additional GCP set up). Please exclude tests marked with integration in the CI before we merge.

Signed-off-by: Rohan Disa <105740583+RohanDisa@users.noreply.github.com>

RohanDisa · 2025-10-12T08:30:02Z

Hey @alxmrs, I have fixed the linting errors. Do tell me if any other changes are needed from my side.

RohanDisa · 2025-10-12T11:09:20Z

I noticed that all five tests failed with the same runner shutdown error:
Error: The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.
This seems to be an infrastructure issue rather than a problem with the code. Please let me know if there’s anything you would like me to check or adjust.

alxmrs · 2025-10-12T11:16:07Z

I just reran the tests. But, reading the logs, it still does look like at least one test is failing.

Do tests pass locally? If so, I wonder what the differences between that and CI are. Would you like assistance trying to get the tests to pass?

RohanDisa · 2025-10-18T18:39:02Z

Hey @alxmrs! Extremely sorry for the late reply. I had exams this week.

Quick summary:
Fixed the chunk boundary logic in test_explode_data_equal_one_last
One memory test (test_read_xarray_loads_one_chunk_at_a_time) fails due to system-dependent thresholds
Excluded the ERA5 integration test since it requires internet and large data

I would appreciate your help in getting the tests to pass!

alxmrs · 2025-10-18T22:45:08Z

I think I know what's going on: the code fails the lint check due to something minor, which happens fast. This cancels all the other tests to run, which is why it appears the tests are failing. Try running:

uvx pyink .

In addition, I think you need to modify the test runner to exclude integration tests, i.e.:

        run: uv run pytest -m "not integration"

In the last line of the ci.yaml.

I've demonstrated this in #84.

- Update CI configuration to exclude integration tests - Fix code formatting with pyink - All unit tests now passing (25/26 tests pass, 1 skipped)

alxmrs · 2025-10-19T16:43:44Z

Merged. Thank you so much, @RohanDisa!

alxmrs reviewed Sep 28, 2025

View reviewed changes

alxmrs approved these changes Oct 2, 2025

View reviewed changes

RohanDisa and others added 3 commits October 2, 2025 13:51

Update xarray_sql/df_test.py

850f914

Co-authored-by: Alex Merose <alex@openathena.ai> Signed-off-by: Rohan Disa <105740583+RohanDisa@users.noreply.github.com>

Removed df_integrationtest.py. Moved the only test in df_test with th…

4601847

…e pytest markers.

Merge branch 'main' of https://github.com/RohanDisa/xarray-sql

3a1d472

alxmrs reviewed Oct 3, 2025

View reviewed changes

RohanDisa added 2 commits October 12, 2025 01:15

Fix linting: reformat code with black

5af5bbe

Merge branch 'main' into main

efea693

Signed-off-by: Rohan Disa <105740583+RohanDisa@users.noreply.github.com>

Fix linting: apply pyink formatter

af8a0f1

Fixed the chunk boundary logic in test_explode_data_equal_one_last

546b88c

Fix pytest conversion issues

10411ca

- Update CI configuration to exclude integration tests - Fix code formatting with pyink - All unit tests now passing (25/26 tests pass, 1 skipped)

alxmrs merged commit 599f86f into xqlsystems:main Oct 19, 2025
7 checks passed

alxmrs mentioned this pull request Nov 25, 2025

Migrate tests to use pytest #74

Closed

		@@ -1,415 +0,0 @@
		<?xml version="1.0" standalone="no"?><!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd"><svg version="1.1" width="1200" height="2266" onload="init(evt)" viewBox="0 0 1200 2266" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:fg="http://github.com/jonhoo/inferno"><!--Flame graph stack visualization. See https://github.com/brendangregg/FlameGraph for latest version, and http://www.brendangregg.com/flamegraphs.html for examples.--><!--NOTES: --><defs><linearGradient id="background" y1="0" y2="1" x1="0" x2="0"><stop stop-color="#eeeeee" offset="5%"/><stop stop-color="#eeeeb0" offset="95%"/></linearGradient></defs><style type="text/css">

		from .df import explode, read_xarray, block_slices, from_map, pivot, from_map_batched


		# -------------------------


		[tool.setuptools_scm]

		[tool.pytest.ini_options]

Uh oh!

Conversation

RohanDisa commented Sep 28, 2025

Uh oh!

alxmrs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alxmrs commented Sep 28, 2025

Uh oh!

RohanDisa commented Oct 1, 2025

Uh oh!

alxmrs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alxmrs commented Oct 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RohanDisa commented Oct 12, 2025

Uh oh!

RohanDisa commented Oct 12, 2025

Uh oh!

alxmrs commented Oct 12, 2025

Uh oh!

RohanDisa commented Oct 18, 2025

Uh oh!

alxmrs commented Oct 18, 2025

Uh oh!

Uh oh!

alxmrs commented Oct 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants