Some test modernization #425
Conversation
and inherit from our testing class
I biffed a couple things on the first pass
|
Ok, so I actually did not put much effort into modernizing the sphinx and vasp tests. I think they could probably benefit greatly from |
Pull Request Test Coverage Report for Build 1534426928
💛 - Coveralls |
I'm not totally happy with this. There is some serious hijinks going on when we explicitly instantiate a class compared to when we create it with one of the tool kits and the JobType wrapper. Everything winds up in the right place (because the underlying ProjectHDFio are the same) but the actual project instance was getting decoupled. I fought with this for waaay too long and now I give up and just manually reconnect the project instance -- everything works beautifully after that. We really need to do some deep dives in pyiron_base though, because the code is riddled with docstrings that say `project` arguments are type `Project`, but they're lying and the type is actually `ProjectHDFio`. I'm not fixing that in this PR though.
Remove duplicate code
Test runners handle things fine without this
I'm the one who wrote them, so it seems fair
CI complains about it. I don't have any idea why as it is totally there in base, but whatever.
|
@samwaseda you were saying earlier that you wanted to spend more time reviewing unit tests, right? 😜 |
|
I'm also a little sketched that the diff is so big. A lot of that looks like it should already be in master from #424 and other recent PRs...Maybe there is some refresh on github's side that I need to figure out? |
# Conflicts: # pyiron_atomistics/atomistics/structure/neighbors.py
|
@samwaseda bump |
|
|
||
| for i, struct in enumerate(job.iter_structures()): | ||
| # breakpoint() | ||
| self.assertTrue(np.allclose(job.output.positions[i], struct.positions)) | ||
| self.assertTrue(np.allclose(job.output.cells[i], struct.cell.array)) | ||
| self.assertTrue(np.allclose(job.output.indices[i], struct.indices)) | ||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
Since the tests don't work in the terminal I'd like to keep these ones
There was a problem hiding this comment.
Ah, sorry, I didn't see this in the mobile app.
Anyhow, it really necessary? I guess if you do python tests/atomistics/job/test_atomistic.py it is, but if you do python -m unittest discover tests/atomistics/job it will run this and the other ones there... but maybe you can append /test_atomistic.py to get just these? Tbh I haven't played around with it much from command line, but I'm 99% sure you can swing it this way
There was a problem hiding this comment.
Yes, with python -m unittest path/to/test_file.py you run all tests in this file (If you point to one specific one, you do not need to discover). So Sam, just make an alias run_test_py="python -m unittest discover " and you may fire any directory you want by run_test_py test_dir :)
😝 well I don't trust me, but the tests are passing and that gives me some confidence, haha |
A bunch of tests weren't passing on my macbook, so I'm fixing that and simultaneously updating them to use more modern syntax (mostly the custom test case classes from base and
state).