Simplify testing with pytest by wil93 · Pull Request #1233 · cms-dev/cms · GitHub

In this PR we:

get rid of cmsRunUnitTests.py entirely, replacing it with pytest which has a powerful 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
fix a bunch of errors that came up while testing with pytest, it seems that our unittest runner was missing some tests because not all of them are executable (should we chmod +x them? I'm almost tempted to chmod -x them, considering that pytest doesn't have this requirement)
fix a bunch of warnings of deprecated methods (e.g. assertEquals -> assertEqual) which I saw in the output thanks to pytest highlighting them (there are still a few more warnings but can't be fixed as easily yet)
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)
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)

Codecov Report

Merging #1233 (50fca65) into master (4baeee8) will increase coverage by 6.10%.
The diff coverage is 77.58%.

❗ Current head 50fca65 differs from pull request most recent head 1723366. Consider uploading reports for the commit 1723366 to get more accurate results

@@            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

Flag	Coverage Δ
functionaltests	47.36% <80.00%> (+0.10%)	⬆️
unittests	56.81% <77.58%> (+12.80%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cms/db/types.py	100.00% <ø> (ø)
cmstestsuite/RunFunctionalTests.py	0.00% <0.00%> (ø)
...mstestsuite/unit_tests/cmscommon/mimetypes_test.py	100.00% <ø> (ø)
setup.py	0.00% <ø> (ø)
cms/io/web_service.py	87.50% <100.00%> (+0.26%)	⬆️
cms/server/contest/server.py	86.36% <100.00%> (ø)
cms/server/contest/submission/workflow.py	95.89% <100.00%> (+0.02%)	⬆️
cmstaskenv/Test.py	17.21% <100.00%> (+17.21%)	⬆️
cmstestsuite/Test.py	52.71% <100.00%> (ø)
cmstestsuite/testrunner.py	15.69% <100.00%> (ø)
... and 7 more

... and 105 files with indirect coverage changes

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.

It's a bit suspicious codecov says this line (and other similar ones) are not covered by tests.

Indeed. I'm looking into it.

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.

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.

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.

Indeed. I'm looking into it.

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.

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

wil93 requested a review from stefano-maggiolo May 2, 2023 21:58

stefano-maggiolo reviewed May 8, 2023

View reviewed changes

wil93 force-pushed the silent_tests branch from 7e363bf to cf174c7 Compare May 13, 2023 10:41

wil93 commented May 13, 2023

View reviewed changes

wil93 force-pushed the silent_tests branch 3 times, most recently from 70dbe1e to 50fca65 Compare May 13, 2023 12:55

stefano-maggiolo approved these changes May 13, 2023

View reviewed changes

wil93 added 4 commits May 14, 2023 12:21

Fix test failures

86b6e54

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).

Fix deprecation warnings surfaced by pytest output

7762ec9

wil93 force-pushed the silent_tests branch from 50fca65 to 2790de2 Compare May 14, 2023 10:21

Improve documentation on how to run tests

1723366

Also update gitignore / dockerignore

wil93 force-pushed the silent_tests branch from 2790de2 to 1723366 Compare May 14, 2023 10:22

wil93 merged commit 3896952 into cms-dev:master May 14, 2023

wil93 deleted the silent_tests branch May 14, 2023 10:30

-                      super().setUp()
-                      file_cacher = FileCacher(path="fs-storage")
-                      self._setUp(file_cacher)
+                      super().setUp(FileCacher(path="fs-storage"))

-                      super().setUp()
-                      file_cacher = FileCacher(path="fs-storage")
-                      self._setUp(file_cacher)
+                      super().setUp(FileCacher(path="fs-storage"))

-                      super().setUp()
-                      file_cacher = FileCacher(path="fs-storage")
-                      self._setUp(file_cacher)
+                      super().setUp(FileCacher(path="fs-storage"))

Simplify testing with pytest by wil93 · Pull Request #1233 · cms-dev/cms · GitHub

Uh oh!

Simplify testing with pytest - #1233

Simplify testing with pytest#1233wil93 merged 5 commits intocms-dev:mastercms-dev/cms:masterfrom wil93:silent_testswil93/cms:silent_testsCopy head branch name to clipboard

Conversation

wil93 commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stefano-maggiolo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stefano-maggiolo May 8, 2023

Choose a reason for hiding this comment

Uh oh!

wil93 May 13, 2023

Choose a reason for hiding this comment

Uh oh!

wil93 May 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefano-maggiolo May 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wil93 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wil93 May 13, 2023

Choose a reason for hiding this comment

Uh oh!

stefano-maggiolo left a comment

Choose a reason for hiding this comment

Uh oh!

stefano-maggiolo May 13, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Simplify testing with pytest#1233
wil93 merged 5 commits into
cms-dev:masterfrom
wil93:silent_tests

wil93 commented May 2, 2023 •
edited
Loading

codecov Bot commented May 2, 2023 •
edited
Loading

wil93 May 13, 2023 •
edited
Loading