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: make NREL EFS download tests faster #148

Merged
merged 1 commit into from
Feb 17, 2021
Merged

Conversation

lanesmith
Copy link
Collaborator

Purpose

The goal of this PR is to speed up the NREL EFS data download and extraction tests. Due to the size of the .zip files, the tests take minutes to run (as opposed to the other tests that take seconds). See #145 for reference.

What the code is doing

test_get_efs_data.py was updated to not test download_demand_data and download_flexibility_data individually, but rather to test the individual private functions (e.g., _download_data, _extract_data) that comprise them. While testing the private functions create some more tests, they are much faster because _download_data can be tested with a much smaller file (the same can't be done with download_demand_data and download_flexibility_data since they are created to only download and extract the large NREL EFS data sets).

Where to look

All of the changes are contained within PreREISE/prereise/gather/demanddata/nrel_efs/tests/test_get_efs_data.py.

Time estimate

This should only take a few minutes.

@lanesmith lanesmith self-assigned this Feb 17, 2021
@danielolsen
Copy link
Contributor

Tests are indeed much faster, thanks for this quick PR. I have some comments on structure but the functional changes look good.

@lanesmith lanesmith linked an issue Feb 17, 2021 that may be closed by this pull request
@lanesmith lanesmith force-pushed the lane/shorter_tests branch 2 times, most recently from 5d99dd0 to 1f140ed Compare February 17, 2021 18:22
Comment on lines 85 to 90
dummy_demand_df.to_csv("test_demand.csv", index=False)

# Create a .zip file of the dummy demand data set
with zipfile.ZipFile("test.zip", "w") as z:
z.write("test_demand.csv")
os.remove("test_demand.csv")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also enclose these file writes within a try/except block as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that "test_demand.csv" will get cleaned up either way below, does "test.zip" get covered as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, test.zip should be cleaned up within _extract_data. I can add an except FileNotFoundError in between the try and finally blocks of test_extract_data in case something goes wrong though.

Can you explain why the above code should be within a try/except block? I don't expect that code to raise an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see about the test.zip deletion.

I'm thinking about what happens if e.g. test.zip fails to be written (e.g. in case another program already has it open). Then we get an exception, and "test_demand.csv" is not cleaned up. I guess it's not too big of a problem though, since whoever is running the tests will hopefully fix the problem and re-run and then the file should get cleaned up properly.

Copy link
Contributor

@danielolsen danielolsen left a comment

Choose a reason for hiding this comment

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

Tests are ~10x faster and seem to still test all the main functions, thanks!

@lanesmith lanesmith merged commit adde7cf into develop Feb 17, 2021
@lanesmith lanesmith deleted the lane/shorter_tests branch February 17, 2021 20:22
@ahurli ahurli mentioned this pull request Mar 16, 2021
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.

Tests are very slow due to big file downloads
2 participants