Typing files from scripts folder #16493

aasiffaizal · 2022-11-05T13:43:27Z

Overview

This PR fixes or fixes part of #[].
This PR does the following: This PR covers type annotation for the files scripts/linters/pre_commit_linter, scripts/linters/python_linter, scripts/run_backend_tests.py and their test files.

Essential 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 linter/Karma presubmit checks have passed locally on your machine.
"Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
- This lets reviewers restart your CircleCI tests for you.
The PR is made from a branch that's not called "develop".

Proof that changes are correct

Proof of changes on desktop with slow/throttled network

Proof of changes on mobile phone

Proof of changes in Arabic language

PR Pointers

Make sure to follow the instructions for making a code change.
If you need a review or an answer to a question, and don't have permissions to assign people, leave a comment like the following: "{{Question/comment}} @{{reviewer_username}} PTAL". Oppiabot will help assign that person for you.
For what code owners will expect, see the Code Owner's wiki page.
Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays.
Never force push. If you do, your PR will be closed.
Some of the e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.

oppiabot · 2022-11-05T13:43:28Z

Hi @aasiffaizal, can you complete the following:

The body of this PR is missing the required description, please update the body with a description of what this PR does.
The karma and linter checklist has not been checked, please make sure to run the frontend tests and lint tests before pushing.
Thanks!

oppiabot · 2022-11-05T13:43:39Z

Hi, @aasiffaizal, this pull request does not have a "CHANGELOG: ..." label as mentioned in the PR pointers. Assigning @aasiffaizal to add the required label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks!

aasiffaizal · 2022-11-06T19:47:11Z

scripts/linters/pre_commit_linter.py

    """Print summary of linter error messages.

    Args:
        lint_messages: list(str). List of linter error messages.
    """
-    if lint_messages != '':


lint_messages is a list of str and hence this condition is not necessary

Should we check if the lint_messages is non-empty?

Added that check

aasiffaizal · 2022-11-06T19:48:31Z

scripts/run_backend_tests.py

@@ -367,6 +397,8 @@ def check_test_results(tasks, task_to_taskspec, generate_coverage_report):
            )

            try:
+                if not tests_failed_regex_match:


Raising an attribute error since there is an exception handler below to handle if there are no matches

Please add some description to the errors, I know it is going to be lost when we handle it but it still make sense for readability.

Added the description

No I meant do Exception('<description>')

aasiffaizal · 2022-11-06T19:50:55Z

@vojtechjelinek @sahiljoster32 @U8NWXD Could you, please take a look?

sahiljoster32

LGTM!

oppiabot · 2022-11-10T07:42:04Z

Unassigning @sahiljoster32 since they have already approved the PR.

vojtechjelinek

Thanks! Just a few nits.

scripts/linters/codeowner_linter.py

scripts/linters/pre_commit_linter.py

vojtechjelinek · 2022-11-10T08:56:05Z

scripts/linters/pre_commit_linter.py

@@ -532,11 +578,11 @@ def _get_task_output(lint_messages, failed, task):
    return failed


-def _print_errors_stacktrace(errors_stacktrace):
+def _print_errors_stacktrace(errors_stacktrace: List[Optional[str]]) -> None:


Why can the items be None?

It can be None because the type of stack_trace is as follows

oppia/scripts/concurrent_task_utils.py

Line 106 in 58d1033

self.stacktrace: Optional[str] = None

It is appended to the list over here

oppia/scripts/concurrent_task_utils.py

Line 162 in 58d1033

ALL_ERRORS.append(task.stacktrace)

should I append to the list only if the stacktrace is present?

Hmm, I would say that if we have task.exception but no task.stacktrace then we probably have some kind of issue and we should print something else instead of the stacktrace.

Yeah that makes sense.

done

vojtechjelinek · 2022-11-10T09:04:24Z

scripts/run_backend_tests.py

@@ -367,6 +397,8 @@ def check_test_results(tasks, task_to_taskspec, generate_coverage_report):
            )

            try:
+                if not tests_failed_regex_match:


No I meant do Exception('<description>')

oppiabot · 2022-11-10T19:08:02Z

Hi @aasiffaizal, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks!

aasiffaizal · 2022-11-11T01:27:11Z

@vojtechjelinek, Addressed all the comments. PTAL!

vojtechjelinek

Thanks! Took a pass.

scripts/run_backend_tests.py

aasiffaizal · 2022-11-12T14:15:04Z

Addressed all the comments @vojtechjelinek. PTAL

vojtechjelinek

LGTM!

U8NWXD · 2022-11-21T17:52:23Z

@aasiffaizal it looks like this PR introduced a backend flake (documented in #16600). Can you investigate? We can't do a clean revert, so if you can fix it fast, that might be faster than reverting manually

aasiffaizal · 2022-11-21T20:17:19Z

Replied in the issue. I'll take a look at it

Typed scripts folder

4008fdc

oppiabot bot assigned aasiffaizal Nov 5, 2022

aasiffaizal assigned sahiljoster32 Nov 5, 2022

aasiffaizal requested a review from sahiljoster32 November 5, 2022 13:43

aasiffaizal added 9 commits November 5, 2022 09:45

Fixed lint issues

e058216

Removed covered linters from exclude list

ddd4e66

Fixed type issues

721ff14

typecheck fix

6f25027

Fixed lint checks

483b495

Fixed lint issues

7b9e572

Fixed failing tests

f64bff5

Updated coverage

24018dd

Added run_backend_tests.py

3a60694

aasiffaizal added the PR CHANGELOG: Typing -- @vojtechjelinek label Nov 6, 2022

aasiffaizal added 3 commits November 6, 2022 00:39

Added run_backend_tests.py

d9428c6

Removed ignore comment

d14791e

Removed ignore comment

08220c1

aasiffaizal unassigned sahiljoster32 Nov 6, 2022

aasiffaizal marked this pull request as ready for review November 6, 2022 19:37

aasiffaizal requested a review from a team November 6, 2022 19:37

aasiffaizal requested a review from a team as a code owner November 6, 2022 19:37

aasiffaizal requested a review from U8NWXD November 6, 2022 19:37

aasiffaizal changed the title ~~Typed scripts folder~~ Typing files from scripts folder Nov 6, 2022

aasiffaizal commented Nov 6, 2022

View reviewed changes

aasiffaizal assigned vojtechjelinek, sahiljoster32 and U8NWXD Nov 6, 2022

sahiljoster32 approved these changes Nov 10, 2022

View reviewed changes

oppiabot bot unassigned sahiljoster32 Nov 10, 2022

vojtechjelinek suggested changes Nov 10, 2022

View reviewed changes

vojtechjelinek assigned aasiffaizal and unassigned vojtechjelinek Nov 10, 2022

oppiabot bot added the PR: don't merge - STALE BUILD label Nov 10, 2022

aasiffaizal added 2 commits November 10, 2022 20:17

Merge remote-tracking branch 'upstream/develop' into scripts-typing

3d0b77e

Addressed Vojta's comments

c1efb53

oppiabot bot removed the PR: don't merge - STALE BUILD label Nov 11, 2022

aasiffaizal requested a review from vojtechjelinek November 11, 2022 01:26

aasiffaizal assigned vojtechjelinek and unassigned aasiffaizal Nov 11, 2022

vojtechjelinek suggested changes Nov 11, 2022

View reviewed changes

scripts/run_backend_tests.py Outdated Show resolved Hide resolved

scripts/run_backend_tests.py Outdated Show resolved Hide resolved

vojtechjelinek assigned aasiffaizal and unassigned vojtechjelinek Nov 11, 2022

aasiffaizal added 3 commits November 11, 2022 21:49

Merge remote-tracking branch 'upstream/develop' into scripts-typing

b7220bc

Addressed vojta's comments -> Remove repetitive comments

25eb015

Fixed lint issues

0ffb0a4

aasiffaizal requested a review from vojtechjelinek November 12, 2022 14:14

aasiffaizal assigned vojtechjelinek and unassigned aasiffaizal Nov 12, 2022

vojtechjelinek approved these changes Nov 13, 2022

View reviewed changes

vojtechjelinek merged commit 0bae307 into oppia:develop Nov 13, 2022

U8NWXD mentioned this pull request Nov 21, 2022

Backend test flake: invalid continuation byte #16600

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typing files from scripts folder #16493

Typing files from scripts folder #16493

aasiffaizal commented Nov 5, 2022 •

edited

Loading

oppiabot bot commented Nov 5, 2022

oppiabot bot commented Nov 5, 2022

aasiffaizal Nov 6, 2022

vojtechjelinek Nov 7, 2022

aasiffaizal Nov 8, 2022

aasiffaizal Nov 6, 2022

vojtechjelinek Nov 7, 2022

aasiffaizal Nov 8, 2022

vojtechjelinek Nov 10, 2022

aasiffaizal Nov 11, 2022

aasiffaizal commented Nov 6, 2022

sahiljoster32 left a comment

oppiabot bot commented Nov 10, 2022

vojtechjelinek left a comment

vojtechjelinek Nov 10, 2022

aasiffaizal Nov 11, 2022

vojtechjelinek Nov 11, 2022

aasiffaizal Nov 12, 2022

vojtechjelinek Nov 10, 2022

oppiabot bot commented Nov 10, 2022

aasiffaizal commented Nov 11, 2022

vojtechjelinek left a comment

aasiffaizal commented Nov 12, 2022

vojtechjelinek left a comment

U8NWXD commented Nov 21, 2022

aasiffaizal commented Nov 21, 2022

Typing files from scripts folder #16493

Typing files from scripts folder #16493

Conversation

aasiffaizal commented Nov 5, 2022 • edited Loading

Overview

Essential Checklist

Proof that changes are correct

Proof of changes on desktop with slow/throttled network

Proof of changes on mobile phone

Proof of changes in Arabic language

PR Pointers

oppiabot bot commented Nov 5, 2022

oppiabot bot commented Nov 5, 2022

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

aasiffaizal commented Nov 6, 2022

sahiljoster32 left a comment

Choose a reason for hiding this comment

oppiabot bot commented Nov 10, 2022

vojtechjelinek 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

oppiabot bot commented Nov 10, 2022

aasiffaizal commented Nov 11, 2022

vojtechjelinek left a comment

Choose a reason for hiding this comment

aasiffaizal commented Nov 12, 2022

vojtechjelinek left a comment

Choose a reason for hiding this comment

U8NWXD commented Nov 21, 2022

aasiffaizal commented Nov 21, 2022

aasiffaizal commented Nov 5, 2022 •

edited

Loading