Fixes #5761: --test_target flag will redirect to its test file. #6014

anubhavsinha98 · 2018-12-20T13:10:13Z

Explanation

Fixes #5761 --test_target flag will redirect to its corresponding backend test file. For ex --test_target='core.domain.feedback_services' will be same as --test_target='core.domain.feedback_services_test' .

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 follows the style guide.
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
- 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.

anubhavsinha98 · 2018-12-20T13:13:57Z

#5761

codecov-io · 2018-12-20T14:13:18Z

Codecov Report

Merging #6014 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           develop   #6014   +/-   ##
=======================================
  Coverage     45.3%   45.3%           
=======================================
  Files          520     520           
  Lines        30667   30667           
  Branches      4597    4597           
=======================================
  Hits         13893   13893           
  Misses       16774   16774

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 8a58279...f25ad48. Read the comment docs.

apb7

Hi @anubhavsinha98, ptal!

apb7 · 2018-12-20T14:43:32Z

scripts/backend_tests.py

@@ -240,6 +240,9 @@ def _convert_to_test_target(path):
 def main():
    """Run the tests."""
    parsed_args = _PARSER.parse_args()
+    if parsed_args.test_target is not None:


Merge both the if conditions with this condition.

anubhavsinha98 · 2018-12-20T17:26:06Z

@apb7 , made the changes. PTAL !

lilithxxx

You can display a warning message if the user entered the wrong command as it was suggested here. Otherwise looks good. Thanks :)

anubhavsinha98 · 2018-12-20T21:19:30Z

@lilithxxx ,can you explain what that wrong command would be as I am not able to recognize?

lilithxxx · 2018-12-20T21:23:38Z

The one without _test in it would be the wrong command.

lilithxxx · 2018-12-20T21:30:13Z

scripts/backend_tests.py

@@ -249,7 +249,10 @@ def main():
        raise Exception('The delimiter in test_target should be a dot (.)')

    if parsed_args.test_target:
-        all_test_targets = [parsed_args.test_target]
+        if parsed_args.test_target.endswith('test'):


Um can you make it endswith('_test') so as to make it more precise? Thanks

anubhavsinha98 · 2018-12-21T06:56:58Z

@lilithxxx, adding the warning message and as @seanlip suggested adding a delay of 3 seconds here also.

anubhavsinha98 · 2018-12-21T07:30:33Z

anubhavsinha98 · 2018-12-21T07:56:21Z

@apb7 , @lilithxxx PTAL !

apb7

@anubhavsinha98, just a couple of minor nits. Thanks!

apb7 · 2018-12-21T08:38:23Z

scripts/backend_tests.py

+        else:
+            print ''
+            print '.....................................................'
+            print 'WARNING : test_target flag should contain test file.'


Change this to WARNING : test_target flag should point to the test file.

apb7 · 2018-12-21T08:39:04Z

scripts/backend_tests.py

+            print '.....................................................'
+            print ''
+            time.sleep(3)
+            print 'Redirecting to its corresponding test file.'


Change this to Redirecting to its corresponding test file...

anubhavsinha98 · 2018-12-21T13:28:40Z

Changes done @apb7 , PTAL !

apb7

Looking all good! @lilithxxx, would you like to review the changes? Thanks!

nithusha21 · 2018-12-21T15:45:32Z

scripts/backend_tests.py

+            print ''
+            print '.....................................................'
+            print 'WARNING : test_target flag should point to the test file.'
+            print '.....................................................'


Just a confirmation, do we normally use . or -?

Hmm..I think -. Nice catch, @nithusha21! @anubhavsinha98, can you please change this? Thanks!

Yea I think we use - @anubhavsinha98 Would you change it please?

Oops -- sorry @apb7 you already mentioned it.

Okay, will change it !
Sorry my fault.

lilithxxx

LGTM! Thanks @anubhavsinha98

apb7

Un-approving till this is resolved. Thanks!

anubhavsinha98 · 2018-12-21T18:06:21Z

Yes @apb7 , I am using the same email in Github and CLA.
Actually codician is actually my system name.

apb7

Uh, why this commit?

anubhavsinha98 · 2018-12-21T18:50:24Z

Really sorry for that. I tested it if any issue is occuring with my github mail.

apb7 · 2018-12-21T19:02:25Z

I understand. Please undo the last commit -- in general, do not commit if your PR has been previously approved. Thanks!

anubhavsinha98 · 2018-12-21T19:18:17Z

Sorry for unconvinience @apb7 , corrected the commit.

anubhavsinha98 · 2018-12-21T19:43:26Z

@apb7 , actually I had not set up mail in the git configurations therefore it was showing codician

apb7

Checked with @anubhavsinha98: He has used the same email for signing the CLA as the one used by GitHub. Thanks!

rahgurung · 2019-01-05T10:27:55Z

It breaks when I try to run test from a subclass inside a file, like core.controllers.feedback_test.FeedbackThreadPermissionsTests ends up being core.controllers.feedback_test.FeedbackThreadPermissionsTests_test thus test break.
I am here with a solution:
We can count dots (.) in the test target and redirect only if there are two dots, one after core and then after controllers and redirect only for two dots, three dots would be for sub classes thus no redirection.

rahgurung · 2019-01-05T10:29:42Z

@nithusha21

anubhavsinha98 · 2019-01-06T04:16:51Z

Okay, good job @gurungrahul2. Nice catch.

anubhavsinha98 · 2019-01-06T05:19:38Z

Actually @gurungrahul2 , I was not aware of the fact that we can test the particular subclass also.
Thanks !

Fixes:5761 test_target issue

45f5b85

anubhavsinha98 requested review from apb7, BenHenning and seanlip as code owners December 20, 2018 13:10

anubhavsinha98 mentioned this pull request Dec 20, 2018

While running back-end tests, module name in test target should redirect to its test class #5761

Closed

apb7 changed the title ~~Fixes:5761 --test_target flag will redirect to its test file.~~ Fixes #5761: --test_target flag will redirect to its test file. Dec 20, 2018

apb7 suggested changes Dec 20, 2018

View reviewed changes

Corrected the if

5b5c32b

lilithxxx reviewed Dec 20, 2018

View reviewed changes

Added the warning message

d8ae7f6

apb7 suggested changes Dec 21, 2018

View reviewed changes

Changed the warning message

9defc8e

apb7 approved these changes Dec 21, 2018

View reviewed changes

nithusha21 reviewed Dec 21, 2018

View reviewed changes

changed . to -

f25ad48

lilithxxx approved these changes Dec 21, 2018

View reviewed changes

nithusha21 approved these changes Dec 21, 2018

View reviewed changes

apb7 suggested changes Dec 21, 2018

View reviewed changes

apb7 reviewed Dec 21, 2018

View reviewed changes

anubhavsinha98 force-pushed the test_target branch from 0e39c21 to f25ad48 Compare December 21, 2018 19:14

apb7 approved these changes Dec 22, 2018

View reviewed changes

apb7 merged commit 02995f8 into oppia:develop Dec 22, 2018

rahgurung mentioned this pull request Jan 6, 2019

Fixes error in backend tests redirection when trying to test a specific module within a test file #6054

Merged

6 tasks

Fixes #5761: --test_target flag will redirect to its test file. #6014

Fixes #5761: --test_target flag will redirect to its test file. #6014

Conversation

anubhavsinha98 commented Dec 20, 2018 • edited by apb7 Loading

Explanation

Checklist

anubhavsinha98 commented Dec 20, 2018

codecov-io commented Dec 20, 2018 • edited Loading

Codecov Report

apb7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anubhavsinha98 commented Dec 20, 2018

lilithxxx left a comment

Choose a reason for hiding this comment

anubhavsinha98 commented Dec 20, 2018

lilithxxx commented Dec 20, 2018

Choose a reason for hiding this comment

anubhavsinha98 commented Dec 21, 2018 • edited Loading

anubhavsinha98 commented Dec 21, 2018

anubhavsinha98 commented Dec 21, 2018

apb7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anubhavsinha98 commented Dec 21, 2018

apb7 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

lilithxxx left a comment

Choose a reason for hiding this comment

apb7 left a comment

Choose a reason for hiding this comment

anubhavsinha98 commented Dec 21, 2018 • edited Loading

apb7 left a comment

Choose a reason for hiding this comment

anubhavsinha98 commented Dec 21, 2018

apb7 commented Dec 21, 2018

anubhavsinha98 commented Dec 21, 2018

anubhavsinha98 commented Dec 21, 2018

apb7 left a comment

Choose a reason for hiding this comment

rahgurung commented Jan 5, 2019

rahgurung commented Jan 5, 2019

anubhavsinha98 commented Jan 6, 2019

anubhavsinha98 commented Jan 6, 2019

anubhavsinha98 commented Dec 20, 2018 •

edited by apb7

Loading

codecov-io commented Dec 20, 2018 •

edited

Loading

anubhavsinha98 commented Dec 21, 2018 •

edited

Loading

anubhavsinha98 commented Dec 21, 2018 •

edited

Loading