-
Notifications
You must be signed in to change notification settings - Fork 17
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
Run PTCDA_single example with pytest #228
Run PTCDA_single example with pytest #228
Conversation
ad5047d
to
3ddc1be
Compare
ee8c621
to
2fcfc18
Compare
Ok, with this, I declare a partial win against tests. To achieve it, I had to do two things:
It looks to me that For the Python 3.8 issue, I don't know the origin of the problem. |
For your information, this is the current test coverage:
47 % is not very good, but not catastrophic either. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yakutovicha Do you think it would make more sense to put the example_*.py
scripts under the tests
folder? They don't really add any value as examples for the end user, so they are a kind of clutter. If you put them under tests
then we also wouldn't need to put the underscore in any of the scripts under the examples
folder.
I tried running the test with Python 3.8 locally, and interestingly the segfault only happens when running all tests with pytest. If run individually (with or without pytest), the test passes, so it seems that one of the other test functions is messing up the state somehow. No clue why it's only happening on 3.8, though. |
The rationale behind this is the separation of unit tests from smoke tests. Also, the way we run ppafm will undergo some transformation soon. So, I would not worry about this now. We can revisit tests in the future when we have a clearer idea of where we are going. |
I believe it has to do with the fact that Python loads modules only once (which makes sense). If the same module is used for two different simulations, it creates problems. The This makes the case clear that storing some state variables inside a module is not a good idea. That is why I created #232. That is also why tests are useful in general 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rationale behind this is the separation of unit tests from smoke tests.
Makes sense, but I also kind of think that the examples directory is also not the right place for them. I would maybe put them into their own subdirectory in tests/smoke_tests
or something, but it's not that important, so I will leave it up to you.
fixes #118
supersedes #205