Fixes #6282: Run front-end tests in pre-push hook, only if changes are made in JS files. #6289

anubhavsinha98 · 2019-02-17T22:28:45Z

Explanation

The frontend tests only run, if any changes are made in JavaScript files.

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.

…o develop

seanlip

Thanks @anubhavsinha98! Left some comments.

seanlip · 2019-02-18T01:11:03Z

scripts/pre_push_hook.py

+        filename for filename in files_to_lint if
+        filename.endswith('.js')]
+
+    if js_files_to_check == []:


Replace all this by: return bool(js_files_to_check).

Oh Thanks @seanlip, nice idea. :)

seanlip · 2019-02-18T01:12:06Z

scripts/pre_push_hook.py

@@ -295,6 +295,19 @@ def _install_hook():
        print 'Copied file to .git/hooks directory'


+def run_frontend_tests_status(files_to_lint):


This is not clear: it sounds like the function is supposed to run the tests, but instead it's just doing a check. So maybe call it does_diff_include_js_files() instead.

codecov-io · 2019-02-18T01:13:37Z

Codecov Report

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

@@           Coverage Diff            @@
##           develop    #6289   +/-   ##
========================================
  Coverage    45.41%   45.41%           
========================================
  Files          594      594           
  Lines        30979    30979           
  Branches      4636     4636           
========================================
  Hits         14069    14069           
  Misses       16910    16910

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 626c4fd...e49ed77. Read the comment docs.

anubhavsinha98 · 2019-02-18T13:26:46Z

Hi @seanlip, done with changes. PTAL. Thanks!

apb7 · 2019-02-18T13:59:53Z

scripts/pre_push_hook.py

@@ -295,6 +295,16 @@ def _install_hook():
        print 'Copied file to .git/hooks directory'


+def does_diff_include_js_files(files_to_lint):
+    """Checks if frontend tests needs to be run."""


This doc string seems to be wrong -- the function checks if the diff includes JS files, right?

seanlip

One more comment but otherwise LGTM.

seanlip · 2019-02-18T18:48:05Z

scripts/pre_push_hook.py

@@ -295,6 +295,16 @@ def _install_hook():
        print 'Copied file to .git/hooks directory'


+def does_diff_include_js_files(files_to_lint):
+    """Returns true if diff includes JS files."""


Missing Args, Returns docstrings.

/cc @oppia/dev-workflow-team we should check for these?

Missing Args, Returns docstrings.

/cc @oppia/dev-workflow-team we should check for these?

Yeah, I also think this check should be added, that every function should have Args and Returns docstrings. What do you suggest @apb7?

What about those functions which does not return anything nor has any args? One example of this is here -> https://github.com/oppia/oppia/blob/develop/core/controllers/base.py#L520

Hm, the lint check should be able to distinguish those by looking at the function signature? Not sure if you can detect returns though. Maybe see if the body contains any return statements.

(And yep, if there are no Args, then no "Args:" section is needed in the docstring. Ditto if there's nothing returned.)

@seanlip We have issue #6081 open for this. Thanks

anubhavsinha98 · 2019-02-18T20:20:46Z

Hi @seanlip, changes done. PTAL. Thanks!

seanlip · 2019-02-19T00:18:01Z

scripts/pre_push_hook.py

-    """Returns true if diff includes JS files."""
-
+    """Returns true if diff includes JS files.
+    Args:


Follow existing spacing conventions. There should be a newline above Args and a newline above Returns.

seanlip · 2019-02-19T00:18:31Z

scripts/pre_push_hook.py

-
+    """Returns true if diff includes JS files.
+    Args:
+        files_to_lint: list of files to be linted.


Missing typeinfo. Also, docstrings have a specific punctuation format, please look at other files to see what it is and follow it exactly.

anubhavsinha98 · 2019-02-19T04:18:49Z

Hi @seanlip, changes are done. Thanks!

seanlip

LGTM. Thanks!

nithusha21 · 2019-02-19T09:24:49Z

Hi @anubhavsinha98, could you please add a changelog label (before merging) as mentioned here. Thanks!

anubhavsinha98 · 2019-02-19T10:58:57Z

Hi @anubhavsinha98, could you please add a changelog label (before merging) as mentioned here. Thanks!

Hi @nithusha21 thanks for the info. Added the changelog label.

seanlip · 2019-02-20T04:47:43Z

Hi @apb7, we need to get this merged. Could you please review as codeowner?

Thanks!

apb7

LGTM, thanks @anubhavsinha98 :)

codician and others added 15 commits November 27, 2018 04:28

First

f115e45

First

28b6e0e

Second

c429cae

Third

83f6a9c

Fourth

9cac86f

Merge branch 'develop' of https://github.com/anubhavsinha98/oppia int…

010e82c

…o develop

Merge branch 'develop' of https://github.com/oppia/oppia into develop

ed6db59

Merge branch 'develop' of https://github.com/oppia/oppia into develop

cd0f299

Testing pre_push_hook

a2f8575

Added pre_push_hook feature

f62e8b9

Added pre_push_hook feature

e989e23

Added pre_push_hook feature

0032cbc

Fixed linting

a0cd7b0

Fixed linting

7cfbb1d

Fixed linting

e66993e

anubhavsinha98 requested a review from apb7 as a code owner February 17, 2019 22:28

Removed Excluded

cd38a58

anubhavsinha98 requested review from seanlip and lilithxxx February 17, 2019 22:29

anubhavsinha98 added 2 commits February 18, 2019 03:59

Removed Excluded

df7a3d1

Removed Excluded

57c38e5

seanlip requested changes Feb 18, 2019

View reviewed changes

seanlip mentioned this pull request Feb 18, 2019

Fix #5134: 100% coverage for learner playlist services #6148

Closed

6 tasks

anubhavsinha98 added 2 commits February 18, 2019 18:57

Changed func name

699d562

Changed func name

e0d81fa

apb7 reviewed Feb 18, 2019

View reviewed changes

Changes Done

38dbfff

seanlip reviewed Feb 18, 2019

View reviewed changes

Added Args & Return

b5844a7

seanlip reviewed Feb 19, 2019

View reviewed changes

Changed Docstring

e49ed77

seanlip approved these changes Feb 19, 2019

View reviewed changes

anubhavsinha98 added the CHANGELOG: Development workflow label Feb 19, 2019

apb7 approved these changes Feb 20, 2019

View reviewed changes

apb7 merged commit 6275049 into oppia:develop Feb 20, 2019

anubhavsinha98 deleted the pre_push_hook branch March 15, 2019 07:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #6282: Run front-end tests in pre-push hook, only if changes are made in JS files. #6289

Fixes #6282: Run front-end tests in pre-push hook, only if changes are made in JS files. #6289

anubhavsinha98 commented Feb 17, 2019

seanlip left a comment

seanlip Feb 18, 2019

anubhavsinha98 Feb 18, 2019

anubhavsinha98 Feb 18, 2019

seanlip Feb 18, 2019

anubhavsinha98 Feb 18, 2019

codecov-io commented Feb 18, 2019 •

edited

Loading

anubhavsinha98 commented Feb 18, 2019

apb7 Feb 18, 2019

anubhavsinha98 Feb 18, 2019

seanlip left a comment

seanlip Feb 18, 2019

anubhavsinha98 Feb 18, 2019

anubhavsinha98 Feb 18, 2019 •

edited

Loading

lilithxxx Feb 19, 2019

seanlip Feb 19, 2019

lilithxxx Feb 19, 2019

anubhavsinha98 commented Feb 18, 2019

seanlip Feb 19, 2019

anubhavsinha98 Feb 19, 2019

seanlip Feb 19, 2019

anubhavsinha98 Feb 19, 2019

anubhavsinha98 commented Feb 19, 2019

seanlip left a comment

nithusha21 commented Feb 19, 2019

anubhavsinha98 commented Feb 19, 2019 •

edited

Loading

seanlip commented Feb 20, 2019

apb7 left a comment

		@@ -295,6 +295,19 @@ def _install_hook():
		print 'Copied file to .git/hooks directory'


		def run_frontend_tests_status(files_to_lint):

Fixes #6282: Run front-end tests in pre-push hook, only if changes are made in JS files. #6289

Fixes #6282: Run front-end tests in pre-push hook, only if changes are made in JS files. #6289

Conversation

anubhavsinha98 commented Feb 17, 2019

Explanation

Checklist

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

codecov-io commented Feb 18, 2019 • edited Loading

Codecov Report

anubhavsinha98 commented Feb 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anubhavsinha98 Feb 18, 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

anubhavsinha98 commented Feb 18, 2019

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

anubhavsinha98 commented Feb 19, 2019

seanlip left a comment

Choose a reason for hiding this comment

nithusha21 commented Feb 19, 2019

anubhavsinha98 commented Feb 19, 2019 • edited Loading

seanlip commented Feb 20, 2019

apb7 left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 18, 2019 •

edited

Loading

anubhavsinha98 Feb 18, 2019 •

edited

Loading

anubhavsinha98 commented Feb 19, 2019 •

edited

Loading