fix: refresh One Building weather data and fix broken URLs #272#273
fix: refresh One Building weather data and fix broken URLs #272#273FedericoTartarini merged 6 commits intodevelopmentfrom
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 ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR updates data maintenance script ignores in ChangesData Maintenance Scripts
URL Extraction & Test Reliability
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
FedericoTartarini
left a comment
FedericoTartarini
left a comment
There was a problem hiding this comment.
Thank you very much for fixing this. This is extremely important. However, I believe that something else has also changed because now, as you can see, the tests are not passing because the top component in the summary page is not loading properly for all the weather files. Would you like me to merge this pull request and then we open another issue for that specific problem? Or would you like to fix it in this pull request? My general preference is not to merge any pull requests that are failing the test.
|
@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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_summary.py (1)
56-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPipeline failure: hard-asserting optional external-API text makes this test flaky.
The pipeline failure (
expected to contain text 'Köppen-Geiger climate zone: …' but actual value did not) is a direct consequence of asserting text thatpages/summary.pyonly renders whenhttp://climateapi.scottpinkelman.comreturns HTTP 200. In CI, that endpoint is either down, rate-limited, or the 5 s timeout fires — causing theexcept Exception: passpath and the text never being rendered. Thenot_to_have_attributewait correctly signals that the Dash callback has finished, but a finished callback with a silenced external-API failure still produces no climate-zone text.Fix options (simplest first):
✅ Option A — skip the climate-zone line from the assertions (least intrusive)
expected_texts = [ "Location: Bologna Marconi AP, ITA", "Longitude: 11.2969", "Latitude: 44.5308", "Elevation above sea level: 37.0 m", "This file is based on data collected between 2004 and 2018", - "Köppen-Geiger climate zone: Cfa. Humid subtropical, no dry season.", "Average yearly temperature: 14.5 °C", "Hottest yearly temperature (99%): 34.0 °C", "Coldest yearly temperature (1%): -2.0 °C", "Annual cumulative horizontal solar radiation: 1546.12 kWh/m2", "Percentage of diffuse horizontal solar radiation: 39.4 %", ]✅ Option B — assert the element is either present with correct text or absent (resilient to API down)
+ climate_locator = info_section.get_by_text("Köppen-Geiger climate zone:", exact=False) + if climate_locator.count() > 0: + expect(climate_locator).to_contain_text("Cfa. Humid subtropical, no dry season.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_summary.py` around lines 56 - 73, The test is flaky because the "Köppen-Geiger climate zone" line is only rendered when the external climate API succeeds; update tests/test_summary.py to not hard-assert that line: either (A) remove the "Köppen-Geiger climate zone: Cfa. Humid subtropical, no dry season." entry from the expected_texts list, or (B) make the assertion for that specific string optional by wrapping the check for that text (the loop that iterates over expected_texts and calls expect(info_section).to_contain_text(text)) in a conditional/try-block so that a missing climate-zone string is tolerated (e.g., try expect(...).to_contain_text(climate_text) and ignore AssertionError), leaving all other expected_texts unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_summary.py`:
- Around line 56-58: The test currently uses
expect(info_section).not_to_have_attribute("data-dash-is-loading","true",...)
which can pass before loading begins; update the test to first wait for the
loading attribute to appear by asserting
expect(info_section).to_have_attribute("data-dash-is-loading","true",
timeout=...) and only after that assert
expect(info_section).not_to_have_attribute("data-dash-is-loading","true",
timeout=...); apply the same two-step pattern to the analogous checks in the
test_unit_switch block (lines referencing info_section in that test) so you wait
for loading to start then for it to finish.
---
Outside diff comments:
In `@tests/test_summary.py`:
- Around line 56-73: The test is flaky because the "Köppen-Geiger climate zone"
line is only rendered when the external climate API succeeds; update
tests/test_summary.py to not hard-assert that line: either (A) remove the
"Köppen-Geiger climate zone: Cfa. Humid subtropical, no dry season." entry from
the expected_texts list, or (B) make the assertion for that specific string
optional by wrapping the check for that text (the loop that iterates over
expected_texts and calls expect(info_section).to_contain_text(text)) in a
conditional/try-block so that a missing climate-zone string is tolerated (e.g.,
try expect(...).to_contain_text(climate_text) and ignore AssertionError),
leaving all other expected_texts unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5328e4f0-cf5b-4034-aa63-6c6b98dffee3
⛔ Files ignored due to path filters (2)
assets/data/OneBuilding files.zipis excluded by!**/*.zipassets/data/one_building.csvis excluded by!**/*.csv
📒 Files selected for processing (4)
.gitignorepages/lib/import_one_building_files.pypages/summary.pytests/test_summary.py
| expect(info_section).not_to_have_attribute( | ||
| "data-dash-is-loading", "true", timeout=20000 | ||
| ) |
There was a problem hiding this comment.
not_to_have_attribute may pass before loading even starts — consider waiting for loading to begin first.
data-dash-is-loading="true" is only set once Dash's callback begins rendering; if the check runs before the attribute is ever attached (i.e. before the callback fires), the assertion passes immediately and subsequent text checks run against incomplete content. A more robust pattern is to first assert the attribute is "true" (i.e. loading started) and then assert it is no longer "true":
🛡️ Proposed fix
- expect(info_section).not_to_have_attribute(
- "data-dash-is-loading", "true", timeout=20000
- )
+ expect(info_section).to_have_attribute(
+ "data-dash-is-loading", "true", timeout=10000
+ )
+ expect(info_section).not_to_have_attribute(
+ "data-dash-is-loading", "true", timeout=20000
+ )The same pattern should be applied to lines 94–96 in test_unit_switch.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(info_section).not_to_have_attribute( | |
| "data-dash-is-loading", "true", timeout=20000 | |
| ) | |
| expect(info_section).to_have_attribute( | |
| "data-dash-is-loading", "true", timeout=10000 | |
| ) | |
| expect(info_section).not_to_have_attribute( | |
| "data-dash-is-loading", "true", timeout=20000 | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_summary.py` around lines 56 - 58, The test currently uses
expect(info_section).not_to_have_attribute("data-dash-is-loading","true",...)
which can pass before loading begins; update the test to first wait for the
loading attribute to appear by asserting
expect(info_section).to_have_attribute("data-dash-is-loading","true",
timeout=...) and only after that assert
expect(info_section).not_to_have_attribute("data-dash-is-loading","true",
timeout=...); apply the same two-step pattern to the analogous checks in the
test_unit_switch block (lines referencing info_section in that test) so you wait
for loading to start then for it to finish.
|
@FedericoTartarini Yes, I noticed the failed tests and will make sure to implement the fix. It seems to be a minor issue and I'll try to get it done ASAP so we can merge. |
|
@FedericoTartarini, the issue was a dead API for Koeppen-Geiger climate zones, causing the location_info test to fail. It also causes an issue in the live version of Clima. I replaced it with a local workaround using the Let me know if you have any questions. |
2 participants
Fixes broken EPW weather file links in the One Building dataset map mentioned in #272.
The issue was initially reported for Singapore (Seletar AP, Changi Intl AP, Paya Lebar) #269 but turned out to be systemic. climate.onebuilding.org had reorganized their file structure, breaking thousands of URLs across all regions.
Rather than patching individual URLs, I refreshed the entire dataset from source:
After regeneration, 1,344 URLs were still broken due to further structural changes on the server.
I created a fix_broken_urls.py script to auto-discover correct paths, covering:
Result: 152 broken links remaining out of 97,926 (0.16%), these files were confirmed absent from the server.
Summary by CodeRabbit
Bug Fixes
Tests
Chores
.gitignorewith additional entries for maintenance scripts.