pymatgen icon indicating copy to clipboard operation
pymatgen copied to clipboard

unittest2pytest pymatgen/core/tests --write --nobackups

Open janosh opened this issue 1 year ago • 7 comments

@shyuep What's your stance on converting pymatgen tests from unittest to pytest?

I find pytest's functional style to be more concise and easier to read than unittest.

janosh avatar Aug 06 '22 02:08 janosh

I do not have a super strong opinion. The only thing is that unittest is included as part of python and I don't find a big problem with it. Pytest is used right now as a dev requirement, but there is no guarantee that we will continue to use it in future.

shyuep avatar Aug 06 '22 02:08 shyuep

but there is no guarantee that we will continue to use it in future.

There were already 18 test files importing pytest prior to this PR. Dropping it from dev deps would take some work.

janosh avatar Aug 06 '22 02:08 janosh

I understand that. But dropping it from 100+ test files would take even more work. We already did a unittest -> nosetests -> pytest switch. I do not know what comes after pytest. At the same time, the default unittest in Python presumably will always be supported for any decent test protocol. PymatgenTest actually inherits from unittest.TestCase and adds support for things like numpy array comparisons and useful structure and test directory tools. I would actually recommend people just use PymatgenTest.

shyuep avatar Aug 06 '22 02:08 shyuep

I see. My bets are on pytest not going anywhere. On GitHub, it has 8x more stars and 4x more repos using it (512k vs 123k) than nose.

The thing with PymatgenTest is many people likely don't know about it. It provides functionality that might be better left to a test framework. E.g. pytest has approx() for array comparison.

janosh avatar Aug 06 '22 03:08 janosh

It wasn't that long ago that I thought nose wouldn't go anywhere too. As for PymatgenTest, I agree some of the functionality are better left to a test framework (e.g., array comparisons, and in fact, PymatgenTest's array comparisons simply delegate to numpy's). But it is not just array comparisons. It is test directory specification, convenience structures, etc.

In any case, I am by no means objecting to pytest. If someone submits new tests in pytest, I will just accept the PR. I just don't think it is a fruitful use of time to migrate old tests based on perfectly fine unittest.TestCase to pytest. Ultimately, the unittests are subject to less stringent code quality criteria than the actual code, e.g., you don't document unittest methods and until black came about, we don't bother with linting quality on tests.

shyuep avatar Aug 06 '22 03:08 shyuep

I just don't think it is a fruitful use of time to migrate old tests based on perfectly fine unittest.TestCase to pytest.

The time investment is actually quite small considering all I did was run unittest2pytest pymatgen/core/tests --write --nobackups and fix a few linter errors. But I see your point. There's still overhead re-running the tests to make sure nothing broke and with reviewing.

janosh avatar Aug 06 '22 15:08 janosh

Coverage Status

Coverage increased (+0.03%) to 63.269% when pulling fcb2a98eafddbc62515290b69fabf54d85a4d167 on janosh:unittest2pytest into dda4cd31af37e1488d3c4f280d96bcda4aeabc97 on materialsproject:master.

coveralls avatar Aug 08 '22 19:08 coveralls

@shyuep I resolved the merge conflicts.

janosh avatar Aug 16 '22 22:08 janosh