Fix #5885 Only run front end test when front end file changed #5936

EvsChen · 2018-12-02T03:45:34Z

Explanation

Fix #5885:
This PR changes the pre_submit_test.sh workflow. If the front end files, i.e. all the files under core/templates/dev/head, not changed, then it skips the front end check.

By default, it compares the local committed changes against the origin/develop branch, but you can also pass your working branch name by the following

bash scripts/run_presubmit_checks.sh --branch=your_branch

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.

codecov-io · 2018-12-02T04:29:49Z

Codecov Report

Merging #5936 into develop will decrease coverage by 0.28%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #5936      +/-   ##
===========================================
- Coverage    45.48%   45.19%   -0.29%     
===========================================
  Files          517      525       +8     
  Lines        30372    31524    +1152     
  Branches      4568     4737     +169     
===========================================
+ Hits         13814    14247     +433     
- Misses       16558    17277     +719

Impacted Files	Coverage Δ
...plates/dev/head/domain/state/StateObjectFactory.js	`81.81% <0%> (-9.49%)`	⬇️
...head/pages/skill_editor/SkillEditorStateService.js	`86.4% <0%> (-5.03%)`	⬇️
...ead/domain/skill/EditableSkillBackendApiService.js	`63.51% <0%> (-3.99%)`	⬇️
...nts/attribution_guide/AttributionGuideDirective.js	`16.66% <0%> (-3.34%)`	⬇️
core/templates/dev/head/app.js	`56.86% <0%> (-2.4%)`	⬇️
...ead/domain/exploration/InteractionObjectFactory.js	`85.29% <0%> (-1.67%)`	⬇️
...d/pages/question_editor/QuestionEditorDirective.js	`2% <0%> (-1.45%)`	⬇️
...exploration_editor/statistics_tab/StatisticsTab.js	`1.4% <0%> (-0.79%)`	⬇️
...es/state_editor/StateInteractionEditorDirective.js	`32.67% <0%> (-0.66%)`	⬇️
...d/pages/skill_editor/SkillEditorNavbarDirective.js	`1.17% <0%> (-0.65%)`	⬇️
... and 22 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 d4a4df4...168f740. Read the comment docs.

seanlip

Thanks @EvsChen! Took a pass, PTAL :)

Also, I suppose the frontend presubmit checks didn't run when you pushed this PR? You might want to do a test by pushing some frontend changes and making sure the presubmit checks run in that case, then reverting them in a subsequent commit -- always a good idea to test the end-to-end flow manually.

scripts/run_presubmit_checks.sh

This reverts commit c66e86b.

EvsChen · 2018-12-03T14:37:20Z

Hi @seanlip, thanks for your advice. Actually using your method I did find a problem. Would it be sufficient to only check against the local develop branch? Otherwise many online changes during the time of the commit would be included.

seanlip · 2018-12-03T18:28:57Z

Actually, shouldn't we compare against "current tip of branch on github"?

seanlip · 2018-12-03T18:29:31Z

Like, if I push commits A, B, C to github, and then prepare a local set of changes in commits D, E, F, G, then on presubmit the question is really whether D/E/F/G contain any frontend changes.

This reverts commit e226e40.

EvsChen · 2018-12-04T14:48:25Z

So the current logic is:
if --branch is set, it's compared with the predefined branch;
else if the current branch has its tip on GitHub, it's comared with its branch tip;
else it's compared with develop

seanlip

Just a couple more questions, thanks!

Could you also confirm you've manually tested the whole flow (as a developer) using various cases? Important to make sure this is done end-to-end, so we don't break things for other folks.

scripts/run_presubmit_checks.sh

This reverts commit d0f8c78.

seanlip · 2018-12-05T22:30:03Z

scripts/run_presubmit_checks.sh

+
+echo "Comparing the current branch with $BRANCH"
+
+if [ -n "$(git diff --cached --name-only --diff-filter=ACM ${BRANCH} | grep ${FRONTEND_DIR})" ]


Ah, take a look at this maybe: https://superuser.com/a/750913

Gives you an explanation of why there are no brackets as well as introducing the -q arg. If you decide to go this route, might be worth referring to this link in a comment.

Apart from that, I think this PR looks good. Once you confirm (by a top-level comment on this PR) that you have manually tested it, it can go in. The manual testing is necessary, I think; there are often hidden issues in bash scripts, and neither you nor I are super familiar with the language, so we need to make sure that this works correctly end-to-end in all cases.

Also, in your comment, please provide a specific list of which scenarios you have manually tested, for future reference -- that way, if this breaks for someone, we can see if it wasn't included here in the first place, or if there's some kind of system-dependent behaviour. Thanks!

EvsChen · 2018-12-08T10:30:47Z

Manually I've tested on the following environments. We may need further tests on windows env but I don't have one at the moment:)

MacOS 10.14 bash & zsh
Vagrant ubuntu/trusty64 bash

DubeySandeep · 2018-12-13T18:29:13Z

Hi @nithusha21, can you help us to find people who can help to test this PR on windows?

nithusha21 · 2018-12-16T07:20:25Z

Ummm, I sent out a message on our gitter chat, for people using windows for development. Didn't get any responses. I don't have access to windows myself either. Not sure if there are any active developers using windows for Oppia development. Do you think the current testing is sufficient in this case? Most of the crowd uses Linux/Mac. If after merging, if we get any error reports, then we would need @EvsChen to investigate :).

DubeySandeep · 2018-12-16T08:11:32Z

@nithusha21, Sounds good to me!
@apb7, do you have any suggestion on this? (If it looks fine to you then can you please approve this PR as a CODEOWNER?)

apb7 · 2018-12-16T08:14:04Z

Hi Sandeep! I am traveling today. I will go through the changes and maybe test it once tomorrow. Thanks! On Dec 16, 2018 1:41 PM, "Sandeep Dubey" <notifications@github.com> wrote: @nithusha21 <https://github.com/nithusha21>, Sounds good to me! @apb7 <https://github.com/apb7>, do you have any suggestion on this? (If it looks fine to you then can you please approve this PR as a CODEOWNER?) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5936 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXrQufiZffChoUxNcisFAR51kd3FwpjTks5u5gA4gaJpZM4Y9OOI> .

apb7

Thanks, @EvsChen!

apb7 · 2018-12-22T20:06:17Z

Dismissing @seanlip's review because of the following:

It has been addressed.
Sean is away at the moment and therefore, will not be able to take a look at this PR.
To merge this PR, we require all approving reviews.

Thanks!

Please see #5936 (comment). Thanks!

Only run front end test when front end file changed

a14a85c

EvsChen requested review from apb7 and seanlip as code owners December 2, 2018 03:45

seanlip previously requested changes Dec 2, 2018

View reviewed changes

scripts/run_presubmit_checks.sh Outdated Show resolved Hide resolved

scripts/run_presubmit_checks.sh Outdated Show resolved Hide resolved

scripts/run_presubmit_checks.sh Outdated Show resolved Hide resolved

scripts/run_presubmit_checks.sh Show resolved Hide resolved

EvsChen added 4 commits December 3, 2018 22:06

Change comments according to review

82d9ce3

Front end file change test

c66e86b

Revert "Front end file change test"

db396d9

This reverts commit c66e86b.

Compare against the local branch

b8407c9

EvsChen added 3 commits December 4, 2018 22:40

Compare with the branch tip on github

c6b5106

Front end file change test

e226e40

Revert "Front end file change test"

ab75d24

This reverts commit e226e40.

seanlip reviewed Dec 5, 2018

View reviewed changes

scripts/run_presubmit_checks.sh Outdated Show resolved Hide resolved

scripts/run_presubmit_checks.sh Outdated Show resolved Hide resolved

EvsChen added 3 commits December 5, 2018 22:46

Front end file change test

d0f8c78

Revert "Front end file change test"

5f79e32

This reverts commit d0f8c78.

Change words

168f740

seanlip reviewed Dec 5, 2018

View reviewed changes

DubeySandeep assigned EvsChen Dec 7, 2018

apb7 approved these changes Dec 22, 2018

View reviewed changes

apb7 merged commit 156ff89 into oppia:develop Dec 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #5885 Only run front end test when front end file changed #5936

Fix #5885 Only run front end test when front end file changed #5936

EvsChen commented Dec 2, 2018 •

edited

Loading

codecov-io commented Dec 2, 2018 •

edited

Loading

seanlip left a comment

EvsChen commented Dec 3, 2018

seanlip commented Dec 3, 2018

seanlip commented Dec 3, 2018

EvsChen commented Dec 4, 2018

seanlip left a comment

seanlip Dec 5, 2018

EvsChen commented Dec 8, 2018

DubeySandeep commented Dec 13, 2018

nithusha21 commented Dec 16, 2018

DubeySandeep commented Dec 16, 2018

apb7 commented Dec 16, 2018 via email

apb7 left a comment

apb7 commented Dec 22, 2018


		echo "Comparing the current branch with $BRANCH"

		if [ -n "$(git diff --cached --name-only --diff-filter=ACM ${BRANCH} \| grep ${FRONTEND_DIR})" ]

Fix #5885 Only run front end test when front end file changed #5936

Fix #5885 Only run front end test when front end file changed #5936

Conversation

EvsChen commented Dec 2, 2018 • edited Loading

Explanation

Checklist

codecov-io commented Dec 2, 2018 • edited Loading

Codecov Report

seanlip left a comment

Choose a reason for hiding this comment

EvsChen commented Dec 3, 2018

seanlip commented Dec 3, 2018

seanlip commented Dec 3, 2018

EvsChen commented Dec 4, 2018

seanlip left a comment

Choose a reason for hiding this comment

seanlip Dec 5, 2018

Choose a reason for hiding this comment

EvsChen commented Dec 8, 2018

DubeySandeep commented Dec 13, 2018

nithusha21 commented Dec 16, 2018

DubeySandeep commented Dec 16, 2018

apb7 commented Dec 16, 2018 via email

apb7 left a comment

Choose a reason for hiding this comment

apb7 commented Dec 22, 2018

EvsChen commented Dec 2, 2018 •

edited

Loading

codecov-io commented Dec 2, 2018 •

edited

Loading