Fix #6888: Run tests split by classes #7040

ankita240796 · 2019-06-28T13:00:37Z

Explanation

Fixes #6888: Splits running backend tests by test classes instead of test files. This will help to speed up the backend tests hopefully (Currently the tests take 3 hours to run, so once backend tests finish running on this PR, we can check if splitting by classes is doing a help here)

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR has an appropriate "CHANGELOG: ..." label (If you are unsure of which label to add, ask the reviewers for guidance).
The PR follows the style guide.
The PR addresses the points mentioned in the codeowner checks for the files/folders changed. (See the codeowner's wiki page.)
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer and don't tick this checkbox.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue. Do not only request the review but also add the reviewer as an assignee.

ankita240796 · 2019-06-28T15:05:12Z

Yes!!! I think this fixes the issue since it took only 30 minutes to complete the backend tests!

apb7 · 2019-06-28T16:21:14Z

Hi @ankita240796, this looks good but is there a way to actually reduce the running time of the test causing the issue? Backend tests usually used to take around 19 minutes on CircleCI. Thanks!

codecov · 2019-06-28T16:21:34Z

Codecov Report

Merging #7040 into develop will decrease coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           develop    #7040     +/-   ##
==========================================
- Coverage    97.46%   97.37%   -0.1%     
==========================================
  Files          379      378      -1     
  Lines        63138    62835    -303     
==========================================
- Hits         61537    61185    -352     
- Misses        1601     1650     +49

Impacted Files	Coverage Δ
core/domain/exp_jobs_one_off_test.py	`100% <100%> (ø)`	⬆️
extensions/answer_summarizers/models_test.py	`99.15% <100%> (ø)`	⬆️
core/jobs.py	`85.28% <0%> (-14.72%)`	⬇️
core/controllers/cron.py	`35.55% <0%> (-0.71%)`	⬇️
core/jobs_test.py	`99.37% <0%> (-0.63%)`	⬇️
core/tests/test_utils.py	`96.63% <0%> (-0.2%)`	⬇️
core/controllers/review_tests.py	`100% <0%> (ø)`	⬆️
core/storage/user/gae_models.py	`100% <0%> (ø)`	⬆️
core/storage/user/gae_models_test.py	`100% <0%> (ø)`	⬆️
core/controllers/review_tests_test.py	`100% <0%> (ø)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cc83ad...a2d0d7a. Read the comment docs.

ankita240796 · 2019-06-28T16:27:06Z

Hi @apb7, I don’t think we can reduce the time further. Since the other fix we can have is to split the test file into multiple test file one per storage model but that will basically be the same as running the test split by classes. The prod jobs test will take some increase in time since the jobs are being run for each test function.

Also, I am not sure why the coverage is being shown as decreased here since I haven’t made changes to the file which are being reported. Will merging develop fix that issue?

Thanks!

apb7 · 2019-06-28T16:35:57Z

Hi @apb7, I don’t think we can reduce the time further. Since the other fix we can have is to split the test file into multiple test file one per storage model but that will basically be the same as running the test split by classes. The prod jobs test will take some increase in time since the jobs are being run for each test function.

Okay, thanks for the explanation. This looks good then.

Also, I am not sure why the coverage is being shown as decreased here since I haven’t made changes to the file which are being reported. Will merging develop fix that issue?

Yes, that should help. /cc @lilithxxx

Thanks!

ankita240796 · 2019-06-28T16:45:47Z

Hi @apb7, I just checked the branch is already updated. It was naming issue in the test files. I have fixed that now, PTAL. Thanks!

apb7 · 2019-06-28T16:49:30Z

core/domain/exp_jobs_one_off_test.py

@@ -2178,7 +2178,7 @@ def test_no_action_is_performed_for_deleted_exploration(self):
            self, exp_jobs_one_off.TranslatorToVoiceArtistOneOffJob)


-class TestDeleteStateIdMappingModelsOneOffJob(test_utils.GenericTestBase):
+class DeleteStateIdMappingModelsOneOffJobTests(test_utils.GenericTestBase):


I think there are other files as well which follow this convention. Can you do a grep for class Test?

Yes, I checked that already but they are mock classes or base classes and are not being used in running the test.

apb7

LGTM, thanks @ankita240796!

seanlip

Thank you, @ankita240796! Left some notes, PTAL.

seanlip · 2019-06-28T18:18:03Z

scripts/backend_tests.py

@@ -211,7 +211,24 @@ def _get_all_test_targets(test_path=None, include_load_tests=True):
    """
    def _convert_to_test_target(path):
        """Remove the .py suffix and replace all slashes with periods."""
-        return os.path.relpath(path, os.getcwd())[:-3].replace('/', '.')
+        classes = []


class_names

seanlip · 2019-06-28T18:20:46Z

scripts/backend_tests.py

@@ -211,7 +211,24 @@ def _get_all_test_targets(test_path=None, include_load_tests=True):
    """
    def _convert_to_test_target(path):
        """Remove the .py suffix and replace all slashes with periods."""


Please update the docstring with the correct args, returns, etc.

Please also change the names of the function / args / variables to reflect what this method is now doing.

seanlip · 2019-06-28T18:22:00Z

scripts/backend_tests.py

+            for line in lines:
+                if line.startswith('class'):
+                    start_index = line.find(' ') + 1
+                    end_index = line.find('Tests(')


I think this is fragile. It just takes someone to not name their class properly -- and then the test won't run, but no one will notice it.

Instead, could you do a check based on what the class is a subclass of? You might be able to use stuff like inspect.getmro (grep for that in the codebase) to do this more semantically, but I think the correct definition of test classes is "is a subclass of the base test class", not "is named XXX".

I actually tried using importlib to import module from the test_target_path but I get errors there as follows:

Checking if node.js is installed in /home/ankita/Desktop/opensrc/oppia_tools Checking whether Google App Engine is installed in /home/ankita/Desktop/opensrc/oppia_tools/google_appengine_1.9.67/google_appengine Checking whether google-cloud-sdk is installed in /home/ankita/Desktop/opensrc/oppia_tools/google-cloud-sdk-251.0.0/google-cloud-sdk Traceback (most recent call last): File "scripts/backend_tests.py", line 395, in <module> main() File "scripts/backend_tests.py", line 290, in main include_load_tests=include_load_tests) File "scripts/backend_tests.py", line 246, in _get_all_test_targets _convert_to_test_target(os.path.join(base_path, root))) File "scripts/backend_tests.py", line 219, in _convert_to_test_target python_module = importlib.import_module(test_target_path) File "/usr/lib/python2.7/importlib/__init__.py", line 37, in import_module __import__(name) File "/home/ankita/Desktop/opensrc/oppia/utils_test.py", line 23, in <module> from core.tests import test_utils File "/home/ankita/Desktop/opensrc/oppia/core/tests/test_utils.py", line 30, in <module> from core.domain import collection_services File "/home/ankita/Desktop/opensrc/oppia/core/domain/collection_services.py", line 33, in <module> from core.domain import exp_services File "/home/ankita/Desktop/opensrc/oppia/core/domain/exp_services.py", line 38, in <module> from core.domain import email_subscription_services File "/home/ankita/Desktop/opensrc/oppia/core/domain/email_subscription_services.py", line 19, in <module> from core.domain import email_manager File "/home/ankita/Desktop/opensrc/oppia/core/domain/email_manager.py", line 22, in <module> from core.domain import config_domain File "/home/ankita/Desktop/opensrc/oppia/core/domain/config_domain.py", line 22, in <module> import schema_utils File "/home/ankita/Desktop/opensrc/oppia/schema_utils.py", line 32, in <module> from core.domain import html_cleaner # pylint: disable=relative-import File "/home/ankita/Desktop/opensrc/oppia/core/domain/html_cleaner.py", line 24, in <module> import bleach ImportError: No module named bleach

Is there some path issues and do I need to insert path for the modules?

Possibly -- see core/tests/gae_suite.py.

But if this becomes too tricky/messy, then perhaps a lexical analysis of the "Abc" in class XYZ(Abc) might be fine -- maybe ensure it is from a given set of strings. That's more fragile though, so better to avoid if possible, but it might be a good middle ground if it proves too hard to use getmro().

I tried adding the paths as in tests/gae_suite.py but I still get errors for modules being imported.

Also, for lexical analysis, the classes should inherit from a fixed class but in many files, we have new base classes and then test classes as a subclass of them. Would it be not better if we could have a test to ensure that all test class names should end in Test/Tests?

Also, for lexical analysis, the classes should inherit from a fixed class but in many files, we have new base classes and then test classes as a subclass of them. Would it be not better if we could have a test to ensure that all test class names should end in Test/Tests?

That could also work, but how would you do that? Such a test would still need to identify which classes are "test classes".

And yeah, that is the drawback of lexical analysis, we'd probably have to maintain a list of "allowed subclasses" or something, but that is indeed icky.

Maybe we should use metaclasses to keep track of subclasses?

This is actually fixed now. I added paths similar to that in core/tests/gae_suite.py. I missed pylint path earlier since it was not present in gae_suite.py but test files like pylint_extensions_tests require pylint import as well and so adding that to path fixed the issue!

seanlip · 2019-06-28T18:23:05Z

(Also, yay on 3 hours --> 30 minutes!!)

ankita240796 · 2019-06-30T07:01:42Z

Hi @seanlip @brianrodri, I have addressed all the review comments. I am sorry for the delay here, I was actually travelling yesterday and was not able to fix this due to that. PTAL, Thanks!

seanlip

Woohoo! Thanks @ankita240796 :) LGTM on the general approach, but I think you'll need to figure out why the backend tests are failing. OK to merge once you've got them to pass. Thanks!

ankita240796 · 2019-06-30T18:53:49Z

Hi @seanlip, I am trying to check the coverage issue locally but I cannot reproduce it. Is it due to some cache issue on ci? Thanks!

seanlip · 2019-06-30T18:57:19Z

TBH, I don't know, sorry...

/cc @apb7 @oppia/dev-workflow-team -- is there a way to clear the cache on CircleCI?

seanlip · 2019-06-30T19:08:05Z

@ankita240796, one thought -- if you can open a separate PR with exactly the same changes as this one, and that PR passes backend tests on CircleCI, then I think I can merge this one. That might be a backup solution...though we should figure out the cache clearing if possible.

ankita240796 · 2019-06-30T19:08:05Z

Hi @YashJipkate, I remember you required clearing the ci cache in one of your PRs. If that's the case, can you please guide me here. Thanks!

ankita240796 · 2019-06-30T19:09:40Z

@ankita240796, one thought -- if you can open a separate PR with exactly the same changes as this one, and comment out the other travis lines except the backend tests, and that PR passes backend tests, then I think I can merge this one. That might be a backup solution...

Sure @seanlip, I will do that now. Thanks!

anubhavsinha98 · 2019-06-30T19:12:49Z

Hi @ankita240796, I am not sure maybe look you can look at https://support.circleci.com/hc/en-us/articles/115015426888-Clear-project-cache

apb7 · 2019-06-30T19:12:51Z

Hi @ankita240796 and @seanlip, I checked out the complete log. This could be caused by cache issues. Ankita, here are the instructions to clear cache on CircleCI. These can be done in this PR only. Ping me in case you have any doubts. Thanks!

ankita240796 · 2019-06-30T19:17:42Z

Hi @anubhavsinha98 @apb7, Thanks for the help. I have commented out the lines to clear the cache and we can wait to see if it works as expected.

ankita240796 · 2019-06-30T19:24:28Z

It still fails. So, I don't think this is a cache issue. I am trying to search if I can find a fix for this. Thanks!

seanlip · 2019-07-01T00:20:01Z

scripts/backend_tests.py

+OPPIA_TOOLS_DIR = os.path.join(CURR_DIR, '..', 'oppia_tools')
+THIRD_PARTY_DIR = os.path.join(CURR_DIR, 'third_party')
+DIRS_TO_ADD_TO_SYS_PATH = [
+    os.path.join(OPPIA_TOOLS_DIR, 'coverage-4.5.3', 'coverage', 'cmdline'),


It looks like this directory cannot be found. I wonder if it is actually being downloaded and installed correctly? Does the backend test script run the setup/install-third-party scripts first?

Maybe you could try doing a listdir to the console to see what's in the folder -- see whether coverage.cmdline exists at all and maybe cat its contents to the terminal.

seanlip

Thanks @ankita240796! Let's get this in to unblock the build, and file an issue for immediate fixing that tries to do this more robustly.

seanlip · 2019-07-01T07:24:50Z

Merging since this passed all checks at ca06f27

Run tests split by classes

027bbbb

ankita240796 added the CHANGELOG: Development workflow label Jun 28, 2019

ankita240796 requested a review from apb7 June 28, 2019 13:00

ankita240796 assigned apb7 Jun 28, 2019

Fix Test/Tests issue

f0d0509

Fix naming

b033436

ankita240796 requested review from brianrodri and DubeySandeep as code owners June 28, 2019 16:45

apb7 reviewed Jun 28, 2019

View reviewed changes

apb7 approved these changes Jun 28, 2019

View reviewed changes

apb7 assigned brianrodri and DubeySandeep and unassigned apb7 Jun 28, 2019

seanlip requested changes Jun 28, 2019

View reviewed changes

ankita240796 added 5 commits June 29, 2019 02:07

Fix names and comments

afb0ece

Merge remote-tracking branch 'upstream/develop' into fix-backend

b8bd46a

Fix docstring

ca06f27

Fix path issue

e311528

Merge remote-tracking branch 'upstream/develop' into fix-backend

915f3d7

ankita240796 assigned seanlip Jun 30, 2019

Add coverage path to avoid circleci failure

7c7b936

seanlip approved these changes Jun 30, 2019

View reviewed changes

seanlip mentioned this pull request Jun 30, 2019

Introduction to Angular 8 #7048

Merged

8 tasks

Try clearing cache

cfe8269

ankita240796 added 7 commits July 1, 2019 00:56

Try fixing coverage issue

9e646da

Check path

3b60d59

Remove extra path

6563422

Remove restore and save cache

238a4b4

Remove print

38a5e32

Try fixing ci

42bbdff

Remove cache comments

5da3a4c

seanlip reviewed Jul 1, 2019

View reviewed changes

Revert to lexical form

a2d0d7a

seanlip approved these changes Jul 1, 2019

View reviewed changes

seanlip removed their assignment Jul 1, 2019

seanlip merged commit f09a67d into oppia:develop Jul 1, 2019

ankita240796 deleted the fix-backend branch July 2, 2019 08:43

ankita240796 mentioned this pull request Jul 3, 2019

Update backend test splitting to use inspection for base classes instead of lexical analysis #7079

Closed

Fix #6888: Run tests split by classes #7040

Fix #6888: Run tests split by classes #7040

Conversation

ankita240796 commented Jun 28, 2019 • edited Loading

Explanation

Checklist

ankita240796 commented Jun 28, 2019

apb7 commented Jun 28, 2019

codecov bot commented Jun 28, 2019 • edited Loading

Codecov Report

ankita240796 commented Jun 28, 2019

apb7 commented Jun 28, 2019

ankita240796 commented Jun 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apb7 left a comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankita240796 Jun 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Jun 28, 2019

ankita240796 commented Jun 30, 2019

seanlip left a comment

Choose a reason for hiding this comment

ankita240796 commented Jun 30, 2019 • edited Loading

seanlip commented Jun 30, 2019

seanlip commented Jun 30, 2019 • edited Loading

ankita240796 commented Jun 30, 2019

ankita240796 commented Jun 30, 2019

anubhavsinha98 commented Jun 30, 2019

apb7 commented Jun 30, 2019

ankita240796 commented Jun 30, 2019

ankita240796 commented Jun 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

seanlip commented Jul 1, 2019

ankita240796 commented Jun 28, 2019 •

edited

Loading

codecov bot commented Jun 28, 2019 •

edited

Loading

ankita240796 Jun 28, 2019 •

edited

Loading

ankita240796 commented Jun 30, 2019 •

edited

Loading

seanlip commented Jun 30, 2019 •

edited

Loading