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

test: include transform profile tests by default #485

Merged
merged 1 commit into from
May 26, 2021
Merged

Conversation

jenhagg
Copy link
Collaborator

@jenhagg jenhagg commented May 20, 2021

Purpose

The transform profile tests don't require the server, but if we run them in CI it would require downloading from blob storage each time, which would significantly increase the wait time and bandwidth required. I uploaded some truncated profiles (just using rows for Jan 1) to blob storage, under a new test_usa_tamu grid model which we can use in the tests with minimal changes.

What the code is doing

Refactor the profile fixtures, point to the new test grid model, add a couple missing pytest markers.

Testing

Ran locally and checked the profiles are downloaded to ~/ScenarioData/raw/test_usa_tamu.

Time estimate

10 min

@BainanXia
Copy link
Collaborator

This is nice. Just curious is it possible to mock a short profile based on the given grid model instead of downloading truncated profiles from blob storage?

@jenhagg jenhagg self-assigned this May 20, 2021
@jenhagg jenhagg added this to the Welcome interns! milestone May 20, 2021
@jenhagg jenhagg linked an issue May 20, 2021 that may be closed by this pull request
@jenhagg
Copy link
Collaborator Author

jenhagg commented May 20, 2021

This is nice. Just curious is it possible to mock a short profile based on the given grid model instead of downloading truncated profiles from blob storage?

Interesting idea, I don't see why not. I think the only requirement is that the plant ids match. It would just be a bit more code to generate the mock profile, and save the csv to the expected location. I originally had the truncated profiles checked into git, but opted for the approach here since it doesn't bypass any functionality or involve any monkeypatching.

@BainanXia
Copy link
Collaborator

BainanXia commented May 20, 2021

It would be great that these tests can run successfully even without internet connection;) Previously, we load real profiles for these tests for the same reason as you mentioned, i.e. that's the most light way (from developer's perspective) to increase the test coverage. As for now, I could see this in two options, a) still load a real grid but mock profiles based on the information of the loaded grid b) mock everything, i.e. starting with a mock grid, then mock profiles accordingly. I presume the second approach is still tricky for now, since we did add lots of new features to the grid which makes it hard to mock. Hence, the first track could be way to proceed I would say.

@jenhagg jenhagg force-pushed the jon/blob_profiles branch from 1a1d3f0 to 3edfb58 Compare May 21, 2021 17:30
@rouille
Copy link
Collaborator

rouille commented May 25, 2021

Not sure what can be done. The TransformProfile class that is tested here instantiates the InputData class and the latter fetch the profiles from the blob storage.

@jenhagg
Copy link
Collaborator Author

jenhagg commented May 25, 2021

One thing to note is that profiles are cached locally, so we would only need an internet connection the first time (though I agree it's better not to require at all). It would be possible to mock the profiles based on the grid, and save them to ~/ScenarioData/raw/test_usa_tamu/ which would bypass the download. However, I also prefer not to duplicate knowledge about the implementation within the tests without a compelling enough reason. So I guess my question would be: is it worth doing in this case? And if so, should it be done as part of this PR?

@BainanXia
Copy link
Collaborator

One thing to note is that profiles are cached locally, so we would only need an internet connection the first time (though I agree it's better not to require at all). It would be possible to mock the profiles based on the grid, and save them to ~/ScenarioData/raw/test_usa_tamu/ which would bypass the download. However, I also prefer not to duplicate knowledge about the implementation within the tests without a compelling enough reason. So I guess my question would be: is it worth doing in this case? And if so, should it be done as part of this PR?

I agree. There is decent amount of work to getting those profile related tests work with mocks which currently rely on real profiles at this point. But the benefits are obvious:

  • This is a one-for-all solution, i.e. once it works for the first time, it can be used everywhere. We can potentially decouple 90% of the tests from real data and internet. We will have a complete mock scenario which can be generated with all essential information filled on the fly. Even the plot functions can be fed with a mock scenario.
  • We don't need to put extra data in blob storage for testing purpose only, i.e. truncated profiles are on longer needed.
  • The complete implementation of the idea, mock implementation + test code refactor, definitely goes beyond the scope of this PR.

@rouille
Copy link
Collaborator

rouille commented May 25, 2021

One final comment I have is that it is complicated to tests the methods of the TransformProfile (scaling and adding) without actually using the same operation (groupby and so on) to build the expected profile

@BainanXia
Copy link
Collaborator

One final comment I have is that it is complicated to tests the methods of the TransformProfile (scaling and adding) without actually using the same operation (groupby and so on) to build the expected profile

It should work if the mock profiles have consistent rows/columns with the same relationship defined in the grid model constants, right? Or we could even have a mock grid model which can be loaded by ModelImmutables.

@jenhagg
Copy link
Collaborator Author

jenhagg commented May 25, 2021

Yeah I was imagining we would just create a time series data frame with the plant ids from the grid (matching the resource type) as columns, and put random values from [0,1] (or some range for demand.csv).

@BainanXia
Copy link
Collaborator

Once we have the mock architecture ready, we could fill it with whatever values are we want as long as it passes basic sanity checks in our code, such as values should be greater than zero etc.

Copy link
Collaborator

@rouille rouille left a comment

Choose a reason for hiding this comment

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

Thanks for improving our test coverage!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include transform profile tests in CI
3 participants