Skip to content

Conversation

@Jiaqi-Lv
Copy link
Collaborator

@Jiaqi-Lv Jiaqi-Lv commented Dec 11, 2025

Overview

This PR addresses issue #975 and introduces two major improvements:


1. Improved Index Usage Warning in SQLiteStore

Problem:
The previous logic for detecting index usage relied only on the first row of EXPLAIN QUERY PLAN. On macOS, this often corresponds to the rtree virtual table, so the check missed actual indexes and always warned.

Solution:

  • Added a new helper method _warn_if_query_not_using_index that inspects all rows of the query plan.
  • The method now looks for any row containing USING INDEX or USING COVERING INDEX (macOS phrasing).
  • If no index is detected, a warning is logged; otherwise, it remains silent.

Impact:
More accurate performance warnings and better developer guidance for adding indexes.


2. Dynamic Port Allocation for Tile Server in Bokeh Tests

Problem:
macOS runs multiple daemons on well-known ports (e.g., 5000). Tests hardcoded to http://127.0.0.1:5000/... often collided with other services, causing HTML/403 responses and subsequent JSONDecodeError or KeyError.

Solution:

  • Introduced dynamic port binding for the tile server:
    • At test session start, a free loopback port is reserved and exported as TIATOOLBOX_TILESERVER_PORT.
    • Both the Flask tile server and Bokeh client read this environment variable, ensuring consistent communication.
  • Updated all relevant tests (test_app_bokeh.py, test_json_config_bokeh.py, test_server_bokeh.py) and CLI launcher to use the dynamic port.
  • Added proxy bypass (trust_env=False) for local requests to avoid interference from system proxies.

Impact:
Eliminates port conflicts, stabilizes UI tests, and improves cross-platform reliability.


Additional Changes

  • Updated Bokeh app hooks and main logic to respect dynamic port configuration.
  • Minor refactoring for clarity and maintainability.

Benefits:

  • More robust query performance checks.
  • Reliable test execution across macOS and CI environments.
  • Cleaner integration between CLI, Bokeh app, and test harness.

Closes: #975

@Jiaqi-Lv Jiaqi-Lv self-assigned this Dec 11, 2025
@Jiaqi-Lv Jiaqi-Lv added bug Something isn't working enhancement New feature or request labels Dec 11, 2025
@Jiaqi-Lv Jiaqi-Lv linked an issue Dec 11, 2025 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Dec 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.37%. Comparing base (aa6c8ab) to head (2df4bf5).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #976      +/-   ##
===========================================
+ Coverage    99.27%   99.37%   +0.09%     
===========================================
  Files           71       71              
  Lines         9161     9176      +15     
  Branches      1195     1197       +2     
===========================================
+ Hits          9095     9119      +24     
+ Misses          40       31       -9     
  Partials        26       26              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@shaneahmed shaneahmed changed the title 🔨 Fix macOS issues 🔨 Fix TileServer issues on macOS Dec 11, 2025
@shaneahmed shaneahmed added this to the Release v2.1.0 milestone Dec 11, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request fixes macOS-specific issues in the TileServer and visualization tests by making the tile server port dynamic and improving SQLite index detection logic.

Key changes:

  • Dynamic port allocation using TIATOOLBOX_TILESERVER_PORT environment variable to avoid port conflicts on macOS
  • Enhanced SQLite query plan inspection to check all rows instead of just the first row, fixing false-positive index warnings on macOS
  • Added proxy bypass configuration for localhost tile server requests

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/conftest.py Reserves a free port at test session start via environment variable
tiatoolbox/visualization/bokeh_app/main.py Updates all hardcoded port references to use dynamic port from environment; adds proxy bypass
tiatoolbox/visualization/bokeh_app/app_hooks.py Updates session destruction hook to use dynamic port
tiatoolbox/cli/visualize.py Updates CLI tile server launcher to use dynamic port
tests/test_app_bokeh.py Adds port variable usage and new tests for app_hooks functionality
tests/test_server_bokeh.py Updates shutdown endpoint to use dynamic port
tests/test_json_config_bokeh.py Updates shutdown endpoint to use dynamic port
tiatoolbox/annotation/storage.py Refactors index warning logic to check entire query plan, fixing macOS false positives

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Jiaqi-Lv Jiaqi-Lv marked this pull request as ready for review December 11, 2025 16:17
Copy link
Member

@shaneahmed shaneahmed left a comment

Choose a reason for hiding this comment

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

Thanks @Jiaqi-Lv

@shaneahmed shaneahmed requested a review from measty December 12, 2025 11:07
Copy link
Collaborator

@measty measty left a comment

Choose a reason for hiding this comment

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

Everything looks good, happy to approve.

@shaneahmed shaneahmed merged commit 59a22d8 into develop Dec 19, 2025
38 of 51 checks passed
@shaneahmed shaneahmed deleted the fix-MacOS-bugs branch December 19, 2025 09:40
YijieZhu15 pushed a commit that referenced this pull request Dec 22, 2025
## Overview
This PR addresses **issue #975** and introduces two major improvements:

---

### 1. Improved Index Usage Warning in `SQLiteStore`
**Problem:**  
The previous logic for detecting index usage relied only on the first row of `EXPLAIN QUERY PLAN`. On macOS, this often corresponds to the `rtree` virtual table, so the check missed actual indexes and always warned.

**Solution:**  
- Added a new helper method `_warn_if_query_not_using_index` that inspects **all rows** of the query plan.
- The method now looks for any row containing `USING INDEX` or `USING COVERING INDEX` (macOS phrasing).
- If no index is detected, a warning is logged; otherwise, it remains silent.

**Impact:**  
More accurate performance warnings and better developer guidance for adding indexes.

---

### 2. Dynamic Port Allocation for Tile Server in Bokeh Tests
**Problem:**  
macOS runs multiple daemons on well-known ports (e.g., `5000`). Tests hardcoded to `http://127.0.0.1:5000/...` often collided with other services, causing `HTML/403` responses and subsequent `JSONDecodeError` or `KeyError`.

**Solution:**  
- Introduced dynamic port binding for the tile server:
  - At test session start, a free loopback port is reserved and exported as `TIATOOLBOX_TILESERVER_PORT`.
  - Both the Flask tile server and Bokeh client read this environment variable, ensuring consistent communication.
- Updated all relevant tests (`test_app_bokeh.py`, `test_json_config_bokeh.py`, `test_server_bokeh.py`) and CLI launcher to use the dynamic port.
- Added proxy bypass (`trust_env=False`) for local requests to avoid interference from system proxies.

**Impact:**  
Eliminates port conflicts, stabilizes UI tests, and improves cross-platform reliability.

---

### Additional Changes
- Updated Bokeh app hooks and main logic to respect dynamic port configuration.
- Minor refactoring for clarity and maintainability.

---

✅ **Benefits:**
- More robust query performance checks.
- Reliable test execution across macOS and CI environments.
- Cleaner integration between CLI, Bokeh app, and test harness.

---

**Closes:** #975


---------

Co-authored-by: Jiaqi Lv <[email protected]>
Co-authored-by: Shan E Ahmed Raza <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test Fails on macOS Tahoe 26.1

4 participants