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

Remove pdf_cache folder before pytest #65

Merged
merged 4 commits into from
Jul 28, 2023
Merged

Remove pdf_cache folder before pytest #65

merged 4 commits into from
Jul 28, 2023

Conversation

dachengx
Copy link
Collaborator

@dachengx dachengx commented Jul 27, 2023

What does the code in this PR do / what does it improve?

blueice makes pdf_cache folder in alea/tests folder when running test.

https://github.com/JelleAalbers/blueice/blob/7c10222a13227e78dc7224b1a7e56ff91e4a8043/blueice/source.py#L73

https://github.com/JelleAalbers/blueice/blob/7c10222a13227e78dc7224b1a7e56ff91e4a8043/blueice/source.py#L106-L109

We should delete the folder because we do not want to reuse the cached sources when testing. Otherwise, some classes like TemplateSource will not be fully tested, because blueice will just use the cached source.

Can you briefly describe how it works?

The pytest fixture allows us to do something before initializing the TestCase.

The configuration of pytest fixture is following https://docs.pytest.org/en/stable/how-to/unittest.html#mixing-pytest-fixtures-into-unittest-testcase-subclasses-using-marks.

Can you give a minimal working example (or illustrate with a figure)?

Just as the pytest goes. Or

coverage run --source=alea -m pytest -s && coverage report

What are the potential drawbacks of the codes?

Please include the following if applicable:

  • Update the docstring(s)
  • Update the documentation
  • Tests to check the (new) code is working as desired.
  • Does it solve one of the open issues on github?

Notes on testing

  • Until the automated tests pass, please mark the PR as a draft.

All italic comments can be removed from this template.

@github-actions
Copy link

github-actions bot commented Jul 27, 2023

Pull Request Test Coverage Report for Build 5695487083

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+5.0%) to 61.283%

Totals Coverage Status
Change from base Build 5682233224: 5.0%
Covered Lines: 573
Relevant Lines: 935

💛 - Coveralls

@dachengx dachengx marked this pull request as ready for review July 27, 2023 16:02
@dachengx dachengx changed the title Remove pdf_cache folder before some tests, remove unused HistogramPdfSource Remove pdf_cache folder before tests, remove unused HistogramPdfSource Jul 27, 2023
@dachengx dachengx requested a review from kdund July 27, 2023 16:03
@kdund
Copy link
Collaborator

kdund commented Jul 27, 2023

The combined template guy is required if we want the safeguard back in and it is useful for mismodelling tests I think it should be kept

@dachengx
Copy link
Collaborator Author

The combined template guy is required if we want the safeguard back in and it is useful for mismodelling tests I think it should be kept

What about SpectrumTemplateSource?

@kdund
Copy link
Collaborator

kdund commented Jul 27, 2023

Less important for current use, but I think we should start using it instead of computing separate signals for each WIMP mass etc :)

@dachengx dachengx changed the title Remove pdf_cache folder before tests, remove unused HistogramPdfSource Remove pdf_cache folder before tests Jul 27, 2023
@kdund
Copy link
Collaborator

kdund commented Jul 27, 2023

BTW, can we rename the folder to alea_test_pdf_cache or sth?
Otherwise, we risk deleting peoples pdf_cache

@dachengx
Copy link
Collaborator Author

dachengx commented Jul 27, 2023

BTW, can we rename the folder to alea_test_pdf_cache or sth? Otherwise, we risk deleting peoples pdf_cache

The PR is only deleting pdf_cache at alea/tests.

@dachengx
Copy link
Collaborator Author

And also the deletion only happens for pytest, not everyone will run pytest locally.

@kdund
Copy link
Collaborator

kdund commented Jul 27, 2023

ah, ok, yes then no prob.

@dachengx dachengx changed the title Remove pdf_cache folder before tests Remove pdf_cache folder before pytest Jul 27, 2023
@dachengx
Copy link
Collaborator Author

@kdund Can we go with this?

@dachengx
Copy link
Collaborator Author

I do not expect the readthedocs action to be passed because we have no docs yet. We can ignore it when merging.

@kdund
Copy link
Collaborator

kdund commented Jul 28, 2023

When I run pytest, I typically run it in the alea base folder-- that one is still there with this PR-- not sure how to best deal with this or if ignoring is best..

@kdund
Copy link
Collaborator

kdund commented Jul 28, 2023

ah, wait-- am I understanding it right that this only deletes the cache before running tests (so not in cleanup?)

@dachengx
Copy link
Collaborator Author

ah, wait-- am I understanding it right that this only deletes the cache before running tests (so not in cleanup?)

Yes. It deletes pdf_cache before the test.

@kdund
Copy link
Collaborator

kdund commented Jul 28, 2023

IMO it should then, if possible also delete it after running the tests
(you can call it from the TestCase TearDown method: tearDown())

@dachengx
Copy link
Collaborator Author

IMO it should then, if possible also delete it after running the tests (you can call it from the TestCase TearDown method: tearDown())

Since not all tests are based on TestCase, I updated the pytest fixture.

Copy link
Collaborator

@kdund kdund left a comment

Choose a reason for hiding this comment

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

For me this now works-- and I understood the doc failure is sth that will be solved post-merge, right, @dachengx ?

@dachengx
Copy link
Collaborator Author

dachengx commented Jul 28, 2023

For me this now works-- and I understood the doc failure is sth that will be solved post-merge, right, @dachengx ?

Yes. It is fixed in #66

@dachengx dachengx merged commit 397d956 into main Jul 28, 2023
@dachengx dachengx deleted the remove_pdf_cache branch July 28, 2023 19:26
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.

2 participants