Unit tests for untested methods of tardis/atomic.py #527
Conversation
- Also, reorganization of import statements and minor PEP8 ixes around data_path.
- Renamed all four tests as 'test_' + 'methodname'.
- Refactored these methods:
- test_read_basic_atom_data
- test_read_ionization_data
- test_read_levels_data
- test_read_lines_data
- These tests comprise of calling the methods and
performing assertions with values of any one row
of obtained astropy.table.Table
- Newly added tests are:
- test_read_synpp_refs
- test_read_zeta_data
- Fixed assert statements in test_read_lines_data.
kdexd
force-pushed
the
test-atomic
branch
2 times, most recently
from
6d4a93b to
e626081
Compare
- Format of the test has been kept similar to the test_read_collision_data() which was added just before this commit.
|
@karandesai-96 I think this is nice to add more coverage. But testing C-functions is much more important to us. Maybe @yeganer can show you what needs to be done there. |
|
@tardis-sn/tardis-core I think this adds more tests to our code. I need one other to sign off or reject this PR. |
|
It came to my knowledge that this code is a new implemented feature to be included in further versions and under heavy development, so writing tests right now might not be good idea as they can break and need to be updated. I have not added tests for scientific code here, only the tests are those which ensure that reading from an hdf5 file is done properly, and most of the part is refactoring the existing tests to cover the raising of exceptions using |
|
|
||
| def test_ionization_h5_readin(): | ||
|
|
||
| def test_read_ionization_data(): | ||
| data = atomic.read_ionization_data(atomic.default_atom_h5_path) |
There was a problem hiding this comment.
I'd suggest using fixtures for the data paths.
That way they could easily be changed for all tests in case something changed.
This goes in line with the DRY principle.
|
If this is signed off, it would be really nice to have some sample data for |
|
@karandesai-96 let me be clear about this: currently there is no need to test this experimental part of tardis as it is still being developed. |
| chianti_he_db_h5_path = os.path.join( | ||
| os.path.dirname(os.path.realpath(__file__)), 'data', 'chianti_he_db.h5') | ||
| @pytest.fixture(scope="module") | ||
| def default_atom_h5_path(): |
There was a problem hiding this comment.
I think It's fine calling atomic.data_path since you are testing that function, too.
|
@wkerzendorf So I would close this PR and keep it mind about this. Over the time this code will be developed and I think we have v1.5 tags floating around here. Once this code gets stable enough, I will reopen it, rebase it and then work on it. It is the best to close it as of now. Sounds like a plan ? |
|
@karandesai-96 - just don't test that part. As far as I can tell there is a lot of good in this. Is there a reason not to merge? |
|
|
||
| def data_path(fname): | ||
| data_dir = os.path.join(os.path.dirname(__file__), 'data') | ||
| return os.path.join(data_dir, fname) | ||
| return os.path.join(os.path.dirname(os.path.realpath(__file__)), 'data', fname) |
There was a problem hiding this comment.
is that more than 140 characters?
There was a problem hiding this comment.
If you are talking about the line length PEP, the value is 80. I'm not sure whether this is too long but line 29 looks like it definitely is to long
There was a problem hiding this comment.
sorry 80 yes. That's I think why I broke it up.
There was a problem hiding this comment.
Is the rule followed as 80 chars or 120 ? Many orgs compromise this PEP8 rule as 120 characters. I'll follow whichever used and push a final cosmetic concluding commit.
There was a problem hiding this comment.
As far as I know we stick with 80. Although we won't do PEP fixes on their own. If I stumble upon code that doesn't follow PEP I'll edit it on the fly.
There was a problem hiding this comment.
I'll take care about it. But why merge if we know of existent PEP8 errors ? I'll include the fixes to cleanup this PR, if we are going with "edit on the fly" way, I'd better squash the PEP8 fixes into previous single commit.
There was a problem hiding this comment.
I'm trying to look out for PEP8 errors but since I'm not doing a checkout of every PR I have a look at I won't see them (I have a pep checker for my editor only).
There was a problem hiding this comment.
I have a built-in PEP8 checker in Pycharm. I had compromised line wrap to 120 characters, but I switched it to 80 characters just now. I removed PEP8 errors from the part of code in this PR. Rest will be "edited on the fly" 😉
|
I think this should be merged. Wolfgang Kerzendorf [email protected] schrieb am Fr., 1. Apr. 2016
|
- Minor cosmetic changes: wrapping lines within 80 characters for keeping code consistent with PEP8 guidelines.
|
I cannot find more additions here now. If I missed anything, I'll include it as asked. |
|
Maybe just one more comment on PEP8: Every PEP8 fix will show a changed line in the files changed part. So for us to review PRs that come from a very junior person contributing makes it harder to look at the actual part of the code. So we encourage PEP8 changes of existing code for the more senior members. Obviously we want everyone (senior & junior) contributors to stick to PEP8 in general. |
|
Acknowledged. 👍 I will add pep8 complaint code from now, without making PEP8 fixes to existing code. |
This PR will be consisting a series of commits targetting to improve code coverage of tardis/atomic.py.
This checklist maintains the methods imporved in tardis/atomic.py. The commits enclosed in brackets show where these tests were modified/ added.
Methods: