Migrate end-to-end testing framework from Cypress to Playwright #261
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request migrates from Cypress to Playwright testing, introduces a centralized global filter system for month/hour filtering across all pages, refactors UI element IDs from tab-centric naming to scope-based identifiers, removes unused CSS resources, and updates callback signatures across multiple pages to support the new global filter store. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Global Filter UI
participant Store as TOOLS_GLOBAL_FILTER_STORE
participant Page as Page Callback
participant Filter as apply_global_month_hour_filter()
participant Chart as Chart Component
User->>UI: Adjust month/hour sliders
User->>UI: Click "Apply month and hour filter"
UI->>UI: update_global_filter_state()
UI->>Store: Update store data<br/>(month_range, hour_range,<br/>invert_month, invert_hour,<br/>filter_active)
Store->>Page: Trigger on data change
Page->>Filter: apply_global_month_hour_filter(df,<br/>filter_store_data)
Filter->>Filter: Apply month/hour masks<br/>to DataFrame
Filter->>Page: Return filtered df
Page->>Chart: Render with filtered data
Chart->>User: Display updated chart
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
pages/lib/charts_data_explorer.py (1)
21-28: Avoid pandas chained assignment (potential silent bug).Use .loc to set filtered values; current df[var][mask] = None risks SettingWithCopy warnings.
Apply:
- mask = (df[filter_var] < min_val) | (df[filter_var] > max_val) - df[var][mask] = None + mask = (df[filter_var] < min_val) | (df[filter_var] > max_val) + df.loc[mask, var] = None @@ - mask = (df[filter_var] >= max_val) & (df[filter_var] <= min_val) - df[var][mask] = None + mask = (df[filter_var] >= max_val) & (df[filter_var] <= min_val) + df.loc[mask, var] = Nonepages/summary.py (1)
313-345: Fix x/y length mismatch for HDD/CDD when global month filter is active.x is derived from df unique months (can be fewer than 12), while y arrays are always length 12 → ValueError in Plotly.
Use a fixed 12‑month x-axis:
- months = df[Variables.MONTH_NAMES.col_name].unique() + from pages.lib.global_scheme import month_lst + months = month_lst # Jan..Dec in orderAlternatively map 1..12 to names:
[month_lst[i-1] for i in range(1,13)].pages/select.py (1)
120-248: Extract repeated filter state dictionary to a constant.The default global filter state dictionary is duplicated 6 times throughout the
submitted_datacallback:{ "month_range": [1, 12], "hour_range": [0, 24], "invert_month": [], "invert_hour": [], "filter_active": False, }This violates DRY principle and makes maintenance harder. If the default state structure needs to change, all 6 locations must be updated.
Define a constant at the module level:
DEFAULT_GLOBAL_FILTER_STATE = { "month_range": [1, 12], "hour_range": [0, 24], "invert_month": [], "invert_hour": [], "filter_active": False, }Then replace all occurrences in the return statements with:
return ( None, None, True, messages_alert["not_available"], "orange", DEFAULT_GLOBAL_FILTER_STATE.copy(), # Use .copy() to avoid mutation )Note: Use
.copy()to ensure each return gets an independent copy of the dictionary.pages/lib/template_graphs.py (1)
656-663: Critical: Hour filtering uses wrong column name.Line 662 applies hour filtering to the MONTH column instead of the HOUR column:
time_filtering( df_copy, start_hour, end_hour, Variables.MONTH.col_name, target_col # ← Wrong! )This will cause incorrect filtering - hour-based filters will mask data based on month values instead of hour values.
Apply this fix:
time_filtering( df_copy, start_month, end_month, Variables.MONTH.col_name, target_col ) time_filtering( - df_copy, start_hour, end_hour, Variables.MONTH.col_name, target_col + df_copy, start_hour, end_hour, Variables.HOUR.col_name, target_col )pages/outdoor.py (1)
159-195: Harden display-name formatting and add fallbackCurrent replacements can miss variants and may return an empty list (no zeros). Suggest regex-based normalization and a fallback message.
Apply:
@@ - display_names = [] - for col in cols_with_the_highest_number_of_zero: - # Remove utci_ prefix and replace all underscores with spaces - display_name = col.replace("utci_", "UTCI ") - display_name = display_name.replace("_", " ") - display_name = display_name.replace("categories", "Categories") - # Fix specific words that need spaces - display_name = display_name.replace("noSun", "No Sun") - display_name = display_name.replace("noWind", "No Wind") - display_name = display_name.replace("Sun", "Sun") # Keep Sun as is - display_name = display_name.replace("Wind", "Wind") # Keep Wind as is - display_names.append(display_name) - - return f"The Best Weather Condition is: {', '.join(display_names)}" + import re + def _fmt(name: str) -> str: + s = re.sub(r'^utci_', 'UTCI ', name, flags=re.I) + s = s.replace('_', ' ') + s = re.sub(r'\bcategories\b', 'Categories', s, flags=re.I) + s = re.sub(r'\bno\s*sun\b', 'No Sun', s, flags=re.I) + s = re.sub(r'\bno\s*wind\b', 'No Wind', s, flags=re.I) + return s + display_names = [_fmt(c) for c in cols_with_the_highest_number_of_zero] + if not display_names: + return "The Best Weather Condition is: No clear winner" + return f"The Best Weather Condition is: {', '.join(display_names)}"
🧹 Nitpick comments (30)
Pipfile (1)
19-19: Scope and pin pytest-playwright as a dev dependency.Keep test-only deps out of runtime and avoid floating versions.
Apply this diff:
[packages] ... -scipy = "==1.12.0" -pytest-playwright = "*" +scipy = "==1.12.0" [dev-packages] cleanpy = "*" pytest = "*" +pytest-playwright = ">=0.5,<0.6" +# (optional) pin Playwright itself to stabilize browser/protocol pairing +# playwright = ">=1.45,<1.46" bump2version = "*" black = "*" ruff = "*" pre-commit = "*"docs/README.md (1)
37-42: Unify contributor formatting for consistency.Use the same linked-name style for all new entries and fix capitalization (e.g., “Wenshu Lyu”). Optionally add profile links for each entry for parity with prior items.
tests/conftest.py (1)
4-6: Provide default and clear error for missing --base-url.Helps local runs and avoids cryptic None usage.
Apply this diff:
@pytest.fixture(scope="session") -def base_url(pytestconfig): - return pytestconfig.getoption("base_url") +def base_url(pytestconfig) -> str: + url = pytestconfig.getoption("base_url") + if not url: + # Default aligned with CI/docs; make it easy to override + url = "http://127.0.0.1:8080" + return url.github/workflows/python.yml (1)
34-37: Optional: use the official Playwright Action for reliability.It installs browsers and OS deps without sudo prompts.
Use:
- name: Setup Playwright uses: microsoft/playwright-github-action@v1 with: with-deps: trueThen drop the manual “Install Playwright Browsers” step.
docs/contributing/contributing.md (2)
80-111: Clarify formatter: Ruff vs Black.Section says Ruff enforces formatting, then instructs to install/run Black. Pick one to avoid conflicting edits, or mark Black as optional.
Apply this diff to rely on Ruff only:
-Install Black: - -```bash -pipenv install black -``` - -Format your code before committing: - -```bash -black . -``` +Ruff includes a formatter; no need for Black. If your editor doesn’t support Ruff fmt yet, it’s fine to use Black locally, but CI will enforce Ruff’s formatting.
133-144: Testing commands: add OS deps and a startup wait; align on port.
- Use
--with-depsfor Linux devs.- Ensure the server port matches the
--base-url.Apply this diff:
pipenv sync -pipenv run playwright install # Required to install browsers +pipenv run playwright install --with-deps # Installs browsers and OS deps (Linux) -Start the app server: +Start the app server (ensure it listens on 8080 to match the pytest base URL): ```bash -pipenv run python main.py +pipenv run python main.py --port 8080-Then, from the root directory, run:
+Then, from the root directory, wait for the server and run tests:-cd tests - -pipenv run pytest --base-url=http://127.0.0.1:8080 +until curl -fsS http://127.0.0.1:8080 >/dev/null; do sleep 1; done +cd tests && pipenv run pytest --base-url=http://127.0.0.1:8080</blockquote></details> <details> <summary>tests/test_wind.py (1)</summary><blockquote> `40-42`: **Fix incomplete comment.** The comment is grammatically incomplete and should be revised for clarity. Apply this diff: ```diff -def test_wind_rose_texts(page: Page): - """Verify that all Wind Rose description texts""" +def test_wind_rose_texts(page: Page): + """Verify that all Wind Rose description texts are correct"""tests/utils.py (2)
17-20: Hardcoded location string creates brittle test dependency.The assertion checks for a specific location "Bologna Marconi AP, ITA" which is tightly coupled to the
test.epwfile. If the test EPW file is replaced or updated, this assertion will fail.Consider making the location check more flexible or parametrizing the expected location.
Apply this diff to make the check more flexible:
- # Verify that the upload success messages are displayed expect(page.get_by_text("The EPW was successfully loaded!")).to_be_visible() - expect( - page.get_by_text("Current Location: Bologna Marconi AP, ITA") - ).to_be_visible() + # Verify that a location was loaded (any location is valid for tests) + expect(page.get_by_text("Current Location:")).to_be_visible()
23-37: Potential selector issues with special characters in tab names.The string interpolation in the selector at Line 29 could fail if
tab_namecontains special characters like quotes or brackets that have special meaning in CSS selectors.While this is unlikely in the current test suite, consider using a more robust selector approach or escaping special characters.
If this becomes an issue, you can use Playwright's text locator with exact matching:
def open_tab(page: Page, tab_name: str): """ Open a specific tab from the sidebar navigation (default expanded version). Works reliably for Mantine NavLink structure. """ # Find by role and accessible name for more robustness nav_link = page.get_by_role("link", name=tab_name).or_( page.get_by_role("button", name=tab_name) ) nav_link.scroll_into_view_if_needed() expect(nav_link).to_be_visible() nav_link.click()pages/lib/charts_data_explorer.py (2)
89-96: Unify hover month labeling (names vs numbers).Filtered branch uses MONTH; unfiltered uses MONTH_NAMES. Prefer names in both for consistency.
Apply:
- customdata=np.stack( - (df[Variables.MONTH.col_name], df[Variables.DAY.col_name]), - axis=-1, - ), + customdata=np.stack( + (df[Variables.MONTH_NAMES.col_name], df[Variables.DAY.col_name]), + axis=-1, + ),And keep MONTH_NAMES in the unfiltered branch as-is.
Also applies to: 143-146
67-101: Drop redundant inner check.Outer condition already ensures any filtered rows exist; remove the inner if filtered_mask.any() to simplify.
Apply:
- if has_filter_marker and df["_is_filtered"].any(): - filtered_mask = df["_is_filtered"] - if filtered_mask.any(): - original_col = f"_{var}_original" - ... - fig.add_trace(...) + if has_filter_marker and df["_is_filtered"].any(): + filtered_mask = df["_is_filtered"] + original_col = f"_{var}_original" + ... + fig.add_trace(...)tests/test_summary.py (1)
49-61: Either click and verify download, or update the docstring.Current test checks visibility only while claiming “clickable.”
Example to assert download:
with page.expect_download() as dl: page.get_by_role("button", name="Download EPW").click() download = dl.value assert download.suggested_filename.endswith(".epw")pages/summary.py (3)
298-305: Pass a list of columns to the global filter for consistency.Other call sites pass a list; keep API uniform.
Apply:
- df = apply_global_month_hour_filter( - df, global_filter_data, Variables.DBT.col_name - ) + df = apply_global_month_hour_filter( + df, global_filter_data, [Variables.DBT.col_name] + )
317-327: Avoid hard‑coded column names in queries.Use Variables.* to prevent drift if names change.
Apply:
- query_month = "month==" - a = df.query(query_month + str(i) + " and DBT<=" + str(hdd_setpoint))[Variables.DBT.col_name].sub(hdd_setpoint) + query_month = f"{Variables.MONTH.col_name}==" + a = df.query(f"{query_month}{i} and {Variables.DBT.col_name}<={hdd_setpoint}")[Variables.DBT.col_name].sub(hdd_setpoint) @@ - a = df.query(query_month + str(i) + " and DBT>=" + str(cdd_setpoint))[Variables.DBT.col_name].sub(cdd_setpoint) + a = df.query(f"{query_month}{i} and {Variables.DBT.col_name}>={cdd_setpoint}")[Variables.DBT.col_name].sub(cdd_setpoint)
201-204: Trim unintended leading spaces in longitude label.Minor UX nit: extra spaces before “Longitude”.
Apply:
- lon = f" Longitude: {meta[Variables.LON.col_name]}" + lon = f"Longitude: {meta[Variables.LON.col_name]}"tests/test_outdoor.py (1)
40-47: Actually select the option and wait for charts to reload.Currently you locate the option but never click nor wait for callback completion.
Example:
dropdown = page.locator("#outdoor-dropdown") dropdown.click() page.get_by_text("UTCI: Sun & no Wind", exact=True).click() # Wait for Dash loading cycle to finish loading = page.locator('[data-dash-is-loading="true"]') loading.first.wait_for(state="detached", timeout=10000) for sel in ("#utci-heatmap", "#utci-category-heatmap", "#utci-summary-chart"): expect(page.locator(sel)).to_be_visible()pages/t_rh.py (1)
103-117: Global filter integration looks good; consider minor DRY/defensive tweaks.
- Factor target_columns into a shared helper/constant to avoid duplication.
- If stores can ever serialize df to dict, defensively coerce to DataFrame as done in summary.py.
Also applies to: 152-158, 217-233, 276-286
pages/natural_ventilation.py (1)
225-246: Move imports to module level and extract duplicate logic.The dynamic imports from
pages.lib.layoutinside the callback are repeated in bothnv_heatmapandnv_bar_chartcallbacks. This pattern has several drawbacks:
- Import overhead on every callback execution
- Code duplication across callbacks
- Harder to maintain consistency
Consider these improvements:
- Move the imports to the top of the file alongside other imports
- Extract the global filter checking and state extraction into a shared helper function
Apply this diff to move imports to module level:
from pages.lib.utils import ( title_with_tooltip, generate_chart_name, generate_units_degree, generate_units, generate_custom_inputs_nv, determine_month_and_hour_filter, title_with_link, ) +from pages.lib.layout import ( + apply_global_month_hour_filter, + get_global_filter_state, +)Then extract a helper function (add after imports):
def get_filter_ranges(global_filter_data): """Extract month and hour ranges from global filter data or return defaults.""" if global_filter_data and global_filter_data.get("filter_active", False): filter_state = get_global_filter_state(global_filter_data) month_range = filter_state["month_range"] hour_range = filter_state["hour_range"] invert_month_global = filter_state["invert_month"] invert_hour_global = filter_state["invert_hour"] return determine_month_and_hour_filter( month_range, hour_range, invert_month_global, invert_hour_global ) else: return 1, 12, 0, 24Then replace lines 225-246 and similar code in
nv_bar_chartwith:start_month, end_month, start_hour, end_hour = get_filter_ranges(global_filter_data) if global_filter_data and global_filter_data.get("filter_active", False): df = apply_global_month_hour_filter( df, global_filter_data, Variables.DBT.col_name )tests/test_t_rh.py (1)
56-68: Test assertion may be brittle and non-specific.The test relies on finding the text "-40" (with
exact=False) to verify the Global/Local toggle worked. This approach has several issues:
- The
-40value might appear in multiple places on the pageexact=Falsemakes the assertion even less specific (matches "-40.5", "-400", etc.)- The test doesn't explain why "-40" indicates Global mode
- Changes to the data or display format could break this test
Consider using a more specific selector or assertion that:
- Targets a specific element by ID or data-testid
- Checks a more distinctive value or UI state
- Adds a comment explaining what "-40" represents
Example improvement:
def test_banner_unit_switch(page: Page): """ Verify that the banner radio buttons (Global/Local) correctly toggle. """ nav_controls = page.locator("#nav-group-controls") nav_controls.click(force=True) global_button = page.get_by_text("Global", exact=True) global_button.scroll_into_view_if_needed() global_button.wait_for(state="visible") global_button.click(force=True) # Verify Global mode is active by checking a chart displays global range minimum # Global temperature range starts at -40°C chart_element = page.locator("#yearly-chart") expect(chart_element).to_be_visible() # Alternative: Check for a more specific element or attribute that indicates Global modepages/psy-chart.py (1)
187-209: Move imports to module level to improve performance.Similar to other files in this PR, the dynamic imports from
pages.lib.layoutinside the callback create unnecessary overhead on every execution.Move the imports to the module level:
from pages.lib.utils import ( generate_chart_name, generate_units, generate_custom_inputs_psy, determine_month_and_hour_filter, title_with_link, dropdown, ) +from pages.lib.layout import ( + apply_global_month_hour_filter, + get_global_filter_state, +)This is a consistent pattern across the codebase and should be applied uniformly to all callback files.
pages/lib/template_graphs.py (1)
542-664: Extract duplicate heatmap trace creation logic.The
heatmapandheatmap_with_filterfunctions contain nearly identical multi-trace creation logic (lines 558-622 and 419-483 respectively). This duplication makes maintenance harder and increases the risk of inconsistencies.Consider extracting the trace creation logic into a shared helper function:
def _create_heatmap_traces(df, var, var_color, var_unit, range_z, y_values): """Create heatmap traces with optional filtered data overlay.""" fig = go.Figure() has_filter_marker = "_is_filtered" in df.columns if has_filter_marker and df["_is_filtered"].any(): # Create filtered trace # ... existing logic ... # Create normal trace # ... existing logic ... else: # Create single trace # ... existing logic ... return figThen both
heatmapandheatmap_with_filtercan call this helper with appropriate y-values.pages/lib/layout.py (1)
553-604: Inconsistent data format for invert flags creates confusion.The invert flags undergo multiple conversions between boolean and list format:
- UI switches store as boolean (
checkedproperty)- Line 580-581: Converted to list
["invert"]or[]for storage- Line 602-603: Converted back to boolean in
get_global_filter_stateThis inconsistency makes the code harder to understand and maintain. The list format
["invert"]appears to be treating the field like a checklist value, but a simple boolean would be clearer.Consider standardizing on boolean throughout:
def update_global_filter_state( apply_clicks, month_range, hour_range, invert_month, invert_hour, current_data ): if apply_clicks is None: apply_clicks = 0 if apply_clicks > 0: current_data["filter_active"] = True current_data.update( { "month_range": month_range or [1, 12], "hour_range": hour_range or [0, 24], - "invert_month": ["invert"] if invert_month else [], - "invert_hour": ["invert"] if invert_hour else [], + "invert_month": bool(invert_month), + "invert_hour": bool(invert_hour), } ) return current_data def get_global_filter_state(filter_store_data): if not filter_store_data: return { "filter_active": False, "month_range": [1, 12], "hour_range": [0, 24], "invert_month": False, "invert_hour": False, } return { "filter_active": filter_store_data.get("filter_active", False), "month_range": filter_store_data.get("month_range", [1, 12]), "hour_range": filter_store_data.get("hour_range", [0, 24]), - "invert_month": bool(filter_store_data.get("invert_month", [])), - "invert_hour": bool(filter_store_data.get("invert_hour", [])), + "invert_month": filter_store_data.get("invert_month", False), + "invert_hour": filter_store_data.get("invert_hour", False), }This would also require updating the default store data in
create_stores(lines 433-439).pages/wind.py (1)
242-251: Move imports to module level for consistency.The dynamic imports from
pages.lib.layoutshould be moved to module level for consistency with best practices and to avoid import overhead on every callback execution.Add at the top of the file with other imports:
from pages.lib.utils import ( generate_chart_name, generate_units, title_with_link, ) +from pages.lib.layout import apply_global_month_hour_filterThen remove the dynamic import statements from the callbacks (lines 243-245, 284, 313, 466-468).
pages/sun.py (2)
199-211: Optional: import hoist to reduce per-callback import churnIf there’s no circular dependency, move apply_global_month_hour_filter import to module scope; else keep as-is.
Also applies to: 261-279, 316-320, 346-349
314-320: Apply row-removal safeguard for consistency withmonthly_and_cloud_chart
monthly_and_cloud_chartremoves rows marked_is_filtered, butdaily()andupdate_heatmap()don't. This inconsistency could cause calculations indaily_profileandheatmapto include filtered data. Apply the safeguard post-filter in both locations:if global_filter_data and global_filter_data.get("filter_active", False): from pages.lib.layout import apply_global_month_hour_filter df = apply_global_month_hour_filter(df, global_filter_data, var) + if isinstance(df, object) and hasattr(df, "columns") and "_is_filtered" in df.columns: + df = df[~df["_is_filtered"]]Apply to: lines 314–320 (daily) and 345–349 (update_heatmap).
pages/outdoor.py (1)
220-247: DRY up repeated global-filter state extractionThe month/hour/invert extraction repeats across three callbacks. Consider a small helper (e.g., resolve_filter_state(global_filter_data)) returning time_filter, month, hour, invert flags.
Happy to factor this into pages/lib/layout.py if desired.
Also applies to: 307-336, 394-421
tests/test_filter.py (2)
145-171: Stabilize baseline and hashing to reduce flakiness
- Set the filter to a known baseline before capturing base_hash.
- Prefer a stable digest over built-in hash().
Apply:
@@ def assert_chart_changes_by_three_steps(page: Page, chart_selector: str): - base_hash = _chart_state_hash(page, chart_selector) + # Ensure a known baseline before hashing + apply_filter(page, BASELINE_MONTH, BASELINE_HOUR) + base_hash = _chart_state_hash(page, chart_selector) @@ - assert changed, ( + assert changed, ( f"Chart did not change after filter steps for selector {chart_selector}" )And:
@@ def _chart_state_hash(page: Page, chart_selector: str) -> str: - html = node.inner_html() - return str(hash(html)) + import hashlib + html = node.inner_html() + return hashlib.blake2s(html.encode("utf-8")).hexdigest()
185-189: Add storage clearing to fixture to prevent state leakage between testsThe codebase uses Mantine components (in pages/lib/layout.py, pages/explorer.py, etc.), which provide
useSessionStorageanduseLocalStoragehooks that persist data in the browser. With the default pytest-playwright configuration, the browser context is shared across tests, so sessionStorage/localStorage will persist between test runs on the same context. Add storage clearing to the fixture to ensure isolation:@pytest.fixture(scope="function", autouse=True) def _bootstrap_each(page: Page, base_url: str): page.context.clear_cookies() page.evaluate('sessionStorage.clear(); localStorage.clear();') page.goto(base_url) upload_epw_file(page) yieldpages/explorer.py (2)
469-476: Duplicate docstring line—remove the repeated sentenceThere are two identical docstrings back-to-back.
- """Update the contents of tab size. Passing in the info from the dropdown and the general info.""" - """Update the contents of tab size. Passing in the info from the dropdown and the general info.""" + """Update the contents of tab size. Passing in the info from the dropdown and the general info."""
528-553: Avoid double time-filtering (optional)You filter df via apply_global_month_hour_filter and then also pass a time_filter_info that re-applies ranges inside custom_heatmap. Consider setting time_filter_info to [False, [1,12], [0,24]] after pre-filtering to reduce work.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Pipfile.lockis excluded by!**/*.locktests/node/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (44)
.github/workflows/cypress.yml(0 hunks).github/workflows/python.yml(1 hunks)Pipfile(1 hunks)assets/banner.css(0 hunks)assets/fonts.css(0 hunks)assets/layout.css(0 hunks)docs/README.md(1 hunks)docs/contributing/contributing.md(4 hunks)pages/explorer.py(17 hunks)pages/lib/charts_data_explorer.py(1 hunks)pages/lib/charts_sun.py(5 hunks)pages/lib/global_element_ids.py(5 hunks)pages/lib/layout.py(8 hunks)pages/lib/template_graphs.py(6 hunks)pages/lib/utils.py(1 hunks)pages/natural_ventilation.py(6 hunks)pages/outdoor.py(6 hunks)pages/psy-chart.py(2 hunks)pages/select.py(6 hunks)pages/summary.py(4 hunks)pages/sun.py(6 hunks)pages/t_rh.py(4 hunks)pages/wind.py(5 hunks)tests/conftest.py(1 hunks)tests/node/cypress.config.js(0 hunks)tests/node/cypress/.gitignore(0 hunks)tests/node/cypress/e2e/spec.cy.js(0 hunks)tests/node/cypress/fixtures/example.json(0 hunks)tests/node/cypress/support/commands.js(0 hunks)tests/node/cypress/support/e2e.js(0 hunks)tests/node/package.json(0 hunks)tests/node/test.epw(0 hunks)tests/python/test_utils.py(0 hunks)tests/test_explorer.py(1 hunks)tests/test_filter.py(1 hunks)tests/test_natural_ventilation.py(1 hunks)tests/test_outdoor.py(1 hunks)tests/test_psy-chart.py(1 hunks)tests/test_select.py(1 hunks)tests/test_summary.py(1 hunks)tests/test_sun.py(1 hunks)tests/test_t_rh.py(1 hunks)tests/test_wind.py(1 hunks)tests/utils.py(1 hunks)
💤 Files with no reviewable changes (13)
- assets/banner.css
- tests/node/cypress/fixtures/example.json
- tests/node/cypress/support/e2e.js
- tests/node/cypress/support/commands.js
- assets/layout.css
- tests/node/cypress.config.js
- tests/node/test.epw
- assets/fonts.css
- .github/workflows/cypress.yml
- tests/node/cypress/.gitignore
- tests/node/cypress/e2e/spec.cy.js
- tests/node/package.json
- tests/python/test_utils.py
🔇 Additional comments (18)
.github/workflows/python.yml (1)
38-41: Remove the --port flag suggestion; it's not supported by the app. Port is already hardcoded to 8080.The app does not accept a
--portCLI argument—AppConfig.PORTis hardcoded to 8080 and passed directly toapp.run(). The suggested diff with--port ${CLIMA_PORT}will not work. However, adding a readiness check is still valid to prevent test flakiness, and usingworking-directoryinstead ofcd testsis a good improvement. Revise the suggestion to remove CLI flag handling while keeping the HTTP readiness check and working-directory change.Likely an incorrect or invalid review comment.
tests/test_sun.py (1)
1-62: LGTM! Well-structured Playwright tests.The test suite follows best practices with:
- Clear setup fixture for navigation and EPW upload
- Well-organized test functions covering different aspects (titles, controls, graphs, interactions)
- Appropriate use of Playwright's expect assertions
- Good element selector strategies (IDs and text matching)
tests/test_psy-chart.py (1)
1-42: LGTM! Consistent test implementation.The Psychrometric Chart tests follow the established patterns from other test files with good coverage of UI elements, controls, and interactions.
tests/test_wind.py (1)
1-55: LGTM! Comprehensive wind rose testing.The test suite effectively validates wind rose visualizations and their associated text content. The use of dictionaries for expected text values and loops for checking multiple elements is clean and maintainable.
tests/test_select.py (1)
1-44: LGTM! Complete EPW upload workflow testing.The test suite provides good coverage of the EPW file selection and upload functionality, from initial page load through successful upload and map rendering.
pages/lib/charts_sun.py (1)
36-124: LGTM! Excellent refactoring for clarity.The change from 0-based indexing with
i+1adjustments to explicit 1-based month numbering withrange(1, 13)significantly improves readability. The introduction ofcol_idxas an alias andis_firstderived from it makes the code intent clearer. All data filters and subplot placements have been updated consistently.tests/utils.py (1)
1-37: Useful test utilities with good documentation.The helper functions are well-documented and provide essential functionality for the test suite. The EPW upload verification and tab navigation helpers will reduce duplication across test files.
tests/test_natural_ventilation.py (1)
1-60: LGTM! Thorough testing of Natural Ventilation interactions.The test suite provides excellent coverage including:
- Static element visibility checks
- Interactive controls validation
- State-dependent behavior (condensation checkbox enabling/disabling the dew point filter)
The tests are well-organized and follow established patterns.
pages/lib/utils.py (1)
269-277: No action required—this change improves correctness.The behavioral change from equality check to truthiness is safe and actually fixes a latent bug. The codebase stores filter inversion state as either:
["invert"](non-empty list) when inversion is enabled[](empty list) when disabledFalse(boolean) as fallback defaultThe old condition
invert_month == Variables.INVERT.col_name(string equality) would fail when receiving["invert"]fromupdate_global_filter_state, preventing inversion from working. The new truthiness checkif invert_month and ...correctly handles all three value types and fixes this bug. All callers pass appropriate values fromfilter_statedictionaries.pages/summary.py (1)
268-289: Original review concern is not valid; no action needed.The
apply_global_month_hour_filterfunction is intentionally designed to accept multiple input types. Its implementation includes defensive code that:
- Converts string arguments to lists:
elif isinstance(target_columns, str): target_columns = [target_columns]- Provides sensible default:
if target_columns is None: target_columns = [Variables.DBT.col_name]The mixed calling patterns observed across the codebase (some passing lists, some passing single strings, some passing nothing) are all handled correctly by the function. No consistency enforcement is needed.
Likely an incorrect or invalid review comment.
pages/sun.py (3)
126-136: SUN id migration + wiring looks goodIDs switched to SUN_* consistently and outputs point to the new containers. No functional concerns spotted.
Also applies to: 301-307, 331-337
188-214: Global filter application + safe drop of flagged rowsGood use of apply_global_month_hour_filter with a targeted column set, and guarding with "_is_filtered" removal before aggregations.
258-279: Target column set for sun path is comprehensive; keep an eye on optional varAppending var when not "None" is correct. If var can alias to a derived field, ensure it exists in df before filtering to avoid KeyError.
Run to spot any non-existent var values reaching this callback:
pages/outdoor.py (2)
47-56: OUTDOOR_DROPDOWN migration is cleanRenamed id and defaults/persistence are sensible.
349-369: Index logic is correct; review concern not founded.The trace ordering in
heatmap_with_filteris deterministic:
- When filter markers exist and filtered data is present: filtered trace (index 0), then normal heatmap (index 1)
- Otherwise: single normal heatmap (index 0)
The current
colorbar_index = 1 if len(utci_stress_cat["data"]) > 1 else 0safely applies the colorbar to the correct trace in both cases. The suggested refactor usingtrace.type == "heatmap"is more defensive but unnecessary here since ordering is guaranteed.Likely an incorrect or invalid review comment.
pages/explorer.py (3)
214-245: Explorer id renames and section scaffolding look consistentEXPLORER_* ids replace TAB6_* cleanly; containers and inputs align with tests.
Also applies to: 248-345, 616-632
720-737: Table: good use of _is_filtered guardFiltering then dropping flagged rows prevents empty columns. LGTM.
654-661: Local fallback path is sensibleUsing filter_df_by_month_and_hour with full column set is a safe default when the global filter is inactive.
|
I have submitted the pull request for the newly added automated tests and workflow updates.The GitHub Actions pipeline has successfully passed all tests and checks. Please kindly review the PR when you have a moment. Thank you for your time and feedback. @FedericoTartarini |
|
@Sallie00 could you please pull the changes from |
|
@Sallie00 a few tests are failing |
# Conflicts: # Pipfile # Pipfile.lock # docs/contributing/contributing.md
|
@FedericoTartarini I’ve addressed the previous errors and made the necessary fixes. This PR is now ready for your review. |
|
@Sallie00 I will approve it now since all the test are passing, however, I would like to ask you to look into how we can speed up or parallelise the tests. Currently it takes more than 10 minutes to run the tests, it should ideally not take this long to do that. Could you please look into solutions on how to speed them up? I tried installing pytest-xdist but it not working. Could you please help me with this? |
FedericoTartarini
merged commit bf5a558
into
CenterForTheBuiltEnvironment:development
2 participants
Summary
This pull request replaces the previous Cypress + Pytest testing setup with a new Playwright (Python) based end-to-end (E2E) testing framework.
The goal of this migration is to simplify the testing environment, improve maintainability, and align all E2E testing with our Python-based architecture.
Changes
Framework Migration
Test Suite Restructure
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Testing
Chores