Simplify testing with pytest #1233
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1233 +/- ##
==========================================
+ Coverage 63.48% 69.58% +6.10%
==========================================
Files 233 328 +95
Lines 17152 26195 +9043
==========================================
+ Hits 10889 18229 +7340
- Misses 6263 7966 +1703
Flags with carried forward coverage won't be shown. Click here to find out more.
|
stefano-maggiolo
left a comment
There was a problem hiding this comment.
Please make sure you copy the PR comments in the commit, it is useful to have them there.
Is there a place where we explain how to run tests? If not, consider creating it.
| super().setUp() | ||
| file_cacher = FileCacher(path="fs-storage") | ||
| self._setUp(file_cacher) | ||
| super().setUp(FileCacher(path="fs-storage")) |
There was a problem hiding this comment.
It's a bit suspicious codecov says this line (and other similar ones) are not covered by tests.
There was a problem hiding this comment.
Indeed. I'm looking into it.
There was a problem hiding this comment.
I think this was caused by __test__ = False on the parent class (TestFileCacherBase) being inherited by TestFileCacherDB and TestFileCacherFS.
Once I fixed it I started getting some unrelated error, which made me realize that super(ParentClass).setUp() doesn't work like I though it did 😅 so now I'm using ParentClass.setUp(self) which although ugly seems to work.
There was a problem hiding this comment.
Looking at docs, super() returns (a proxy for) the first superclass in the resolution order; you can specify the starting point but it's even more confusing. So either we keep things like you did, or we add super().setUp() to our mixins, and I think that should work with a simple super().setUp() at line 370 here.
wil93
left a comment
There was a problem hiding this comment.
I split the commits in a more sensible way (BTW: I was anyway planning to squash-merge at the end, and put all the comments in a single commit message, are you OK with it?)
I just added some documentation on how to run tests: I created a "Docker image" section which I plan to expand as soon as we will have a docker-compose.dev.yml file for the development use case.
| super().setUp() | ||
| file_cacher = FileCacher(path="fs-storage") | ||
| self._setUp(file_cacher) | ||
| super().setUp(FileCacher(path="fs-storage")) |
There was a problem hiding this comment.
Indeed. I'm looking into it.
70dbe1e to
50fca65
Compare
stefano-maggiolo
left a comment
There was a problem hiding this comment.
If you can take a look whether changing the super() stuff works easily, please change it here, otherwise let's keep what you have done.
It's ok to submit all in once, but consider splitting the assertEquals change.
| super().setUp() | ||
| file_cacher = FileCacher(path="fs-storage") | ||
| self._setUp(file_cacher) | ||
| super().setUp(FileCacher(path="fs-storage")) |
There was a problem hiding this comment.
Looking at docs, super() returns (a proxy for) the first superclass in the resolution order; you can specify the starting point but it's even more confusing. So either we keep things like you did, or we add super().setUp() to our mixins, and I think that should work with a simple super().setUp() at line 370 here.
Pytest has a nice auto-discovery feature (see pytest.ini file), is compatible with the unittest-style tests we're already using, and produces a far more readable output which makes it easy to see what's failing. This commit removes RunUnitTests.py and RunTests.py, leaving only RunFunctionalTests.py (which could later be migrated to pytest as well) We also rename the 'db' container to 'cms_test_db' to better distinguish it from other locally running containers and in preparation for adding docker-compose.dev.yml later on.
These files weren't running with our test runner because they weren't marked as executable. They now are (although we don't need them to be).
Split `docker build` and `docker run` in two separate workflow steps to reduce the output size in the github action and make it more readable Also improve the docker (re)build speed by better using the cache: we first copy the requirements.txt, then install, then copy the rest of the folder, which means we won't invalidate the cached `pip install` step unless we really need to
Also update gitignore / dockerignore
In this PR we:
cmsRunUnitTests.pyentirely, replacing it withpytestwhich has a powerful auto-discovery feature (seepytest.inifile), is compatible with theunittest-style tests we're already using, and produces a far more readable output which makes it easy to see what's failingpytest, it seems that our unittest runner was missing some tests because not all of them are executable (should wechmod +xthem? I'm almost tempted tochmod -xthem, considering that pytest doesn't have this requirement)assertEquals->assertEqual) which I saw in the output thanks topytesthighlighting them (there are still a few more warnings but can't be fixed as easily yet)docker buildanddocker runin two separate workflow steps (to reduce the output size in the github action and make it more readable)docker (re)buildspeed by better using the cache (we first copy therequirements.txt, then install, then copy the rest of the folder, which means we won't invalidate the cachedpip installstep unless we really need to)