Thanks for pointing this out, @ankita240796! There indeed seems to be some problem here. I suggest we first increase the timeout_multiplier in pre_commit_script.py and then fix the errors so that we know that they will be caught the next time. So I suggest the following here:

1. Undo the fixes temporarily here.
2. In the first commit, increase the timeout multiplier to may be 2000 and see if the lint checks fail (which they should).
3. In the next commit, fix the problems.

(I will be happy to do a PR for this if you have any problem here). Thanks!

Sure @apb7, I will make the changes as suggested by you and update you, Thanks!

Codecov Report

Merging #6038 into develop will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff            @@
##           develop    #6038   +/-   ##
========================================
  Coverage    45.27%   45.27%           
========================================
  Files          521      521           
  Lines        30688    30688           
  Branches      4597     4597           
========================================
  Hits         13894    13894           
  Misses       16794    16794

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 618344f...f2c84cc. Read the comment docs.

Hi @apb7, lint checks fail on increasing the value of timeout_multiplier. I have checked this as well as made the changes on the PR, PTAL! Thanks.

@ankita240796: Great, just a doubt: where did you test that the lint checks fail? Ideally, this should be checked on Travis.

Hi @apb7,

My bad here, I ran the test locally.
Shall I revert the last commit and wait for the travis output?

Thanks!

Yes, that sounds good! We can only wait for the lint checks to finish. Thanks!

@ankita240796: The lint checks still pass. I think the actual problem is not with the timeout multiplier here. It is with how we check whether a test fails.

You might require a bit of debugging through the pre-commit script to reach the root cause and fix this issue, which I think is necessary before we fix the lint errors. Would you like to work on this? If not, no problem! I do have a couple of theories and will be happy to make a PR for this. Thanks!

Hi @apb7, I found the reason for the failure of test. I have also made changes in the PR to fix the issue.

We check if test have failed by checking if any message starts with FAILED (https://github.com/oppia/oppia/blob/develop/scripts/pre_commit_linter.py#L1708).

The way messages were appended to summary_messages here results in appending only the first item from the iterator and the last message which contains the FAILED keyword is left out.

I have made the changes to append the complete list of errors, which should fix the issue.

PTAL, Thanks!

Hi @apb7, I have made the changes as suggested by you, PTAL!

Also the lint test failed for previous commit. Shall I make the fixes now?

Thanks!

@ankita240796: The lint checks indeed fail but I think the JS lint errors are not getting logged here. This might be due to the fact that js_result is still empty when summary_messages are being populated.

I will try to fix that @apb7, Thanks!

@ankita240796, I looked into this a bit and think that the JS errors not getting logged might be a timeout issue. ATM we have 794 JS files out of a total of 1640 files to lint. This gives us a timeout of about 484 s with the multiplier set at 1000. I suggest we set the multiplier to 1500. That might give us a sufficient timeout (around 750 s). What do you suggest here?

Hi @apb7, I think it would be better to go with the option of increasing timeout before looking for other fixes as that may be actually the reason behind the issue. I have made the changes according to that. We can wait now to see the output of travis.

Hi @apb7, the lint test fail but now we get js output separately (https://travis-ci.org/oppia/oppia/jobs/473387172#L2904) but not in summary messages. I feel increasing the timeout a bit further can fix the issue. What do you think?

@ankita240796: Sure, please go ahead and try that out. Thanks!

@ankita240796, the timeout of 2000 seems to workout. Just to double check, can you please do the following:

1. Make a commit to fix only the pylint error and wait for the lint checks to complete. If everything works fine, the lint checks should fail solely due to the js lint errors.
2. Go ahead and fix the js lint errors as well.

Post this, we can go ahead and merge this in! Thanks.

Sorry for the delay here @apb7, I was travelling so was not able to update this. I have made the changes now, PTAL. Thanks!

@ankita240796, no problem, thanks! The lint checks indeed fail. Please go ahead and fix the js lint errors. This will be good to go then!

Done @apb7!