Add automatic logging of e2e flakes, and automatic restarts #10654

nithusha21 · 2020-09-12T15:17:20Z

Overview

This PR fixes or fixes part of N/A
This PR does the following: We track all e2e flakes in a sheet. This PR edits the e2e tests runner to interact with the sheet to automatically detect if a failure is a flake. If so, it will automatically log that in the sheet, and restart the test upto 3 runs. This helps with better management and prioritization of the list of flakes, and automatically restarts, making the flakes less annoying for contributors.

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".

PR Pointers

Make sure to follow the instructions for making a code change.
Oppiabot will notify you when you don't add a PR_CHANGELOG label. If you are unable to do so, please @-mention a code owner (who will be in the Reviewers list), or ask on Gitter.
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.

…-logging

oppiabot · 2020-09-12T15:17:22Z

Hi, @nithusha21, this pull request does not have a "CHANGELOG: ..." label as mentioned in the PR checkbox list. Assigning @nithusha21to 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!

kevintab95 · 2020-09-13T17:59:09Z

Thanks @nithusha21! I don't think my review is required here -- so unassigning myself.

U8NWXD · 2020-09-14T00:07:47Z

@nithusha21 since we still have to fix the backend tests, can this be made a draft?

We need to import `google.oauth2` after we import `python_utils` because importing `python_utils` sets `sys.path` as needed for `google.oauth2`. This commit replicates changes from oppia#11018 that fix the linter to recognize google.oauth2 as a third-party library.

U8NWXD · 2020-10-22T01:59:56Z

If you need this to get in soonish, maybe just cherrypick or redo the changes from #11018 here?

There'll be merge conflicts down the road, but if getting this in is worth it, then that's one way to speed things up.

done in 24adaff

Hudda

LGTM

DubeySandeep

I took a pass and left a few comments. Most of the comments are related to code structure, if required we can resolve that comment after getting this PR merged as this PR will help every other contributors. Let me know if you have any question/suggestion. Thanks!

DubeySandeep · 2020-10-22T02:04:50Z

app_dev.yaml

+- third_party/python_libs/google/oauth2/
+- third_party/python_libs/google/api_core/


We are adding 4 new lib but removing 2 dirs here? Is that expected?

Fixed. The path was incorrect.

DubeySandeep · 2020-10-22T02:06:53Z

requirements.in

@@ -27,3 +27,7 @@ requests-toolbelt==0.9.1
 google-cloud-tasks==1.5.0
 googleapis_common_protos==1.52.0
 webapp2==3.0.0b1
+google-auth==1.21.1


Optional: Nit: Should we add this in sorted order* (Though it's only sorted till line 22)?

DubeySandeep · 2020-10-22T02:08:53Z

requirements.txt

@@ -30,10 +33,12 @@ graphy==1.0.0             # via -r requirements.in, googleappenginemapreduce
 grpc-google-iam-v1==0.12.3  # via google-cloud-tasks
 grpcio==1.32.0            # via google-api-core, googleapis-common-protos, grpc-google-iam-v1
 html5lib==1.0.1           # via -r requirements.in
+httplib2==0.18.1          # via google-api-python-client, google-auth-httplib2


Should we also remove this from the build container? (i.e, add this in -ski-file list of app.yaml)

DubeySandeep · 2020-10-22T02:09:12Z

requirements.txt

 idna==2.10                # via requests
 mock==3.0.5               # via googleappenginemapreduce
 mox==0.5.3                # via googleappenginemapreduce
 mutagen==1.43.0           # via -r requirements.in
+oauthlib==3.1.0           # via requests-oauthlib


Ditto here and elsewhere*

DubeySandeep · 2020-10-22T02:13:05Z

scripts/run_e2e_tests.py

+    os.getcwd(), 'third_party', 'python_libs')
+sys.path.insert(0, _PYTHON_LIBS_PATH)
+
+import googleapiclient.discovery # isort:skip  pylint: disable=wrong-import-position, wrong-import-order


We should move the google spreadsheet related code to a different file? (We can do it later if required but make sure to create an issue for the same!)

Created #11038 to resolve all the below comments as I don't have time right now to perform this refactor. I will do that when I am back.

Thanks for creating ths issue, I'm working on it and I'll create a separate PR.

DubeySandeep · 2020-10-22T02:14:15Z

scripts/run_e2e_tests.py

+    """
+    sheet_id = os.getenv('FLAKY_E2E_TEST_SHEET_ID')
+    flaky_tests_list = []
+    if sheet_id is not None:


if sheet_id is None: return [] restult = ....

[To avoid lots of code inside if branch!]

DubeySandeep · 2020-10-22T02:16:24Z

scripts/run_e2e_tests.py

+
+import googleapiclient.discovery # isort:skip  pylint: disable=wrong-import-position, wrong-import-order
+
+MAXIMUM_RUNS = 3


Maybe MAX_RETRY_COUNT or something around that?

DubeySandeep · 2020-10-22T02:20:57Z

scripts/run_e2e_tests.py

+
+    flaky_tests_list = []
+    google_auth_decode_password = os.getenv('GOOGLE_AUTH_DECODE_PASSWORD')
+    if google_auth_decode_password is not None:


Should we move the code 647-660 to a function get_flkay_test_data_sheet?

DubeySandeep · 2020-10-22T03:19:48Z

scripts/run_e2e_tests.py

+    """Run tests, rerunning at most MAXIMUM_RUNS times if they flake."""
+    for _ in python_utils.RANGE(MAXIMUM_RUNS):
+        flake_state = run_tests(args=args)
+        if flake_state != 'flake':


Should we structure it this way:

get_flkay_test_data_sheet: data = get_flkay_test_data_sheet() return get_flaky_tests_data_from_sheets(data) is_test_output_flaky(output): logs = get_flkay_test_logs() for log in logs: if is_output_similar_to_log(output, log) return True return False main: return_code = 1 Loop max_tries: output, return_code = run_test() if is_test_output_faky(output): update_falkey_test_count() [OR do it this inside the is_test_output_flkay() call] print 'The test result looks flaky, rerunning the tests (%<attempt number>)' else: break sys.exist(return_code)

[This is just to make the code structured and readable!]

oppiabot · 2020-10-22T03:25:19Z

Unassigning @DubeySandeep since the review is done.

vojtechjelinek

LGTM!

oppiabot · 2020-10-22T12:19:21Z

Unassigning @vojtechjelinek since they have already approved the PR.

U8NWXD · 2020-10-23T02:31:38Z

I'm seeing an import error again, but only on TravisCI:

ImportError: No module named oauth2

seanlip · 2020-10-23T07:48:07Z

This works on my local machine (or at least, it gets past the point on Travis where it's currently stuck). Per @kevintab95 I'm going to try closing and reopening this PR in order to (hopefully) break the cache (assuming caching is the issue).

@U8NWXD, if this persists, maybe you could just test out (on a separate branch) what happens for a single Travis test, and maybe add logs as needed to get more insight into what's going on?

into custom-e2e-logging

oppiabot · 2020-10-24T04:07:46Z

Hi @nithusha21. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

oppiabot · 2020-10-26T01:40:38Z

Hi @nithusha21. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

seanlip · 2020-10-28T04:21:34Z

Closing -- superseded by #11056.

nithusha21 added 13 commits September 2, 2020 22:14

add automatic restarts for flakes, and automatic logging in sheets

a9d9163

use service accounts instead of API keys, and download the credentials

aad7995

some lint fixes

44d7bab

add encryption, and fix conditions for checking for flakes

3b23601

Merge branch 'develop' of github.com:nithusha21/oppia into custom-e2e…

42cf32e

…-logging

add python dependencies

92549d1

add more pip dependencies

e6d9ce9

fix lint issues

950d39b

fix more lint issues

59c9203

fix more lint issues

199691e

reorder codeowner files

6a4a842

fix import issues, and add missing lib to installation

cab4b8f

exclude encrypted file from linting

ecb94b7

nithusha21 requested review from kevintab95, DubeySandeep, U8NWXD and krishnarao22 September 12, 2020 15:17

nithusha21 requested review from Hudda and vojtechjelinek as code owners September 12, 2020 15:17

oppiabot bot assigned nithusha21 Sep 12, 2020

nithusha21 added the PR CHANGELOG: Developer UX -- @DubeySandeep label Sep 12, 2020

nithusha21 assigned kevintab95, DubeySandeep, U8NWXD and krishnarao22 and unassigned nithusha21 Sep 12, 2020

kevintab95 removed their assignment Sep 13, 2020

Fix Import Order

24adaff

We need to import `google.oauth2` after we import `python_utils` because importing `python_utils` sets `sys.path` as needed for `google.oauth2`. This commit replicates changes from oppia#11018 that fix the linter to recognize google.oauth2 as a third-party library.

Hudda approved these changes Oct 22, 2020

View reviewed changes

Hudda removed their assignment Oct 22, 2020

DubeySandeep requested changes Oct 22, 2020

View reviewed changes

oppiabot bot unassigned DubeySandeep Oct 22, 2020

DubeySandeep mentioned this pull request Oct 22, 2020

Fix #11006: Add google as known third-party library #11018

Merged

4 tasks

vojtechjelinek approved these changes Oct 22, 2020

View reviewed changes

oppiabot bot unassigned vojtechjelinek Oct 22, 2020

seanlip closed this Oct 23, 2020

seanlip reopened this Oct 23, 2020

Resolve some comments

3a15bd4

nithusha21 mentioned this pull request Oct 23, 2020

Fix remaining comments from #10654 #11038

Closed

Merge branch 'custom-e2e-logging' of https://github.com/nithusha21/oppia

6df2057

into custom-e2e-logging

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Oct 24, 2020

Updated with upstream develop

da11309

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Oct 24, 2020

DubeySandeep approved these changes Oct 24, 2020

View reviewed changes

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Oct 26, 2020

Merge branch 'develop' into custom-e2e-logging

93fc5a8

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Oct 27, 2020

U8NWXD mentioned this pull request Oct 27, 2020

Custom e2e logging 2 #11056

Merged

4 tasks

seanlip closed this Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add automatic logging of e2e flakes, and automatic restarts #10654

Add automatic logging of e2e flakes, and automatic restarts #10654

nithusha21 commented Sep 12, 2020

oppiabot bot commented Sep 12, 2020

kevintab95 commented Sep 13, 2020

U8NWXD commented Sep 14, 2020

U8NWXD commented Oct 22, 2020

Hudda left a comment

DubeySandeep left a comment

DubeySandeep Oct 22, 2020

nithusha21 Oct 23, 2020

DubeySandeep Oct 22, 2020

nithusha21 Oct 23, 2020

DubeySandeep Oct 22, 2020

nithusha21 Oct 23, 2020

DubeySandeep Oct 22, 2020

nithusha21 Oct 23, 2020

DubeySandeep Oct 22, 2020

nithusha21 Oct 23, 2020

DubeySandeep Oct 24, 2020

DubeySandeep Oct 22, 2020

DubeySandeep Oct 22, 2020

nithusha21 Oct 23, 2020

DubeySandeep Oct 22, 2020

DubeySandeep Oct 22, 2020

oppiabot bot commented Oct 22, 2020

vojtechjelinek left a comment

oppiabot bot commented Oct 22, 2020

U8NWXD commented Oct 23, 2020

seanlip commented Oct 23, 2020 •

edited

Loading

oppiabot bot commented Oct 24, 2020

oppiabot bot commented Oct 26, 2020

seanlip commented Oct 28, 2020

		- third_party/python_libs/google/oauth2/
		- third_party/python_libs/google/api_core/


		import googleapiclient.discovery # isort:skip pylint: disable=wrong-import-position, wrong-import-order

		MAXIMUM_RUNS = 3

Add automatic logging of e2e flakes, and automatic restarts #10654

Add automatic logging of e2e flakes, and automatic restarts #10654

Conversation

nithusha21 commented Sep 12, 2020

Overview

Essential Checklist

PR Pointers

oppiabot bot commented Sep 12, 2020

kevintab95 commented Sep 13, 2020

U8NWXD commented Sep 14, 2020

U8NWXD commented Oct 22, 2020

Hudda left a comment

Choose a reason for hiding this comment

DubeySandeep 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Oct 22, 2020

vojtechjelinek left a comment

Choose a reason for hiding this comment

oppiabot bot commented Oct 22, 2020

U8NWXD commented Oct 23, 2020

seanlip commented Oct 23, 2020 • edited Loading

oppiabot bot commented Oct 24, 2020

oppiabot bot commented Oct 26, 2020

seanlip commented Oct 28, 2020

seanlip commented Oct 23, 2020 •

edited

Loading