Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix snapshot tests #231

Merged
merged 1 commit into from
Sep 18, 2024
Merged

Fix snapshot tests #231

merged 1 commit into from
Sep 18, 2024

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Sep 17, 2024

The snapshot tests have been failing for a while now. Initially, if I remember correctly, there was some issue with retrieving the snapshots consistently, but that seems to be resolved. Now, one test fails because it's looking for the PRICE_EMISSION_NEW variable: the legacy reporting we migrated is built on top of https://github.com/iiasa/message_ix/tree/issue/723, which introduced that variable name to fix PRICE_EMISSION.

While we have unfortunately still not finished the corresponding PR, the branch has progressed to a point where PRICE_EMISSION was actually updated instead of a new helper variable introduced. This PR thus adjusts the variable name the migrated legacy reporting expects.

How to review

  • Read the diff and note that the CI checks all pass.
  • Note that the TEMPORARY commit should be dropped before merging

PR checklist

  • Continuous integration checks all ✅
  • Update tests
  • [ ] Add, expand, or update documentation. Just CI.
  • [ ] Update doc/whatsnew. Just CI.

@glatterf42 glatterf42 added bug Something isn't working ci Continuous integration & testing labels Sep 17, 2024
@glatterf42 glatterf42 self-assigned this Sep 17, 2024
Copy link

codecov bot commented Sep 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.5%. Comparing base (ae7b63f) to head (d5665a6).

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #231     +/-   ##
=======================================
+ Coverage   65.5%   66.5%   +0.9%     
=======================================
  Files        203     203             
  Lines      15819   15819             
=======================================
+ Hits       10375   10523    +148     
+ Misses      5444    5296    -148     

see 7 files with indirect coverage changes

@khaeru
Copy link
Member

khaeru commented Sep 17, 2024

A passing thought: in theory, a similar/parallel change should have been made to the legacy reporting somewhere, perhaps on the (private) message_data branch SSP_dev (or whatever the name is) that is directly using the branch from iiasa/message_ix#726. Otherwise it would also break without PRICE_EMISSION_NEW.

Is there someone we can ask to point us there so we can confirm this is aligned?

@glatterf42
Copy link
Member Author

Good point, thanks :)
Here's the corresponding commit from two weeks ago: https://github.com/iiasa/message_data/commit/221b6de7309ec390c7d9981e5c7b554430205d5f

Copy link
Member

@khaeru khaeru left a comment

Choose a reason for hiding this comment

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

Great, thanks for confirming. LGTM.

@glatterf42 glatterf42 force-pushed the fix/test-snapshot-price-emission-new branch from d5665a6 to 815782f Compare September 17, 2024 13:55
@glatterf42 glatterf42 merged commit 4c9cf32 into main Sep 18, 2024
26 checks passed
@glatterf42 glatterf42 deleted the fix/test-snapshot-price-emission-new branch September 18, 2024 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ci Continuous integration & testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants