Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add new flag "download_combined_frontend_spec_file" to frontend test execution file #15635

Merged
merged 2 commits into from Jun 29, 2022
Merged

Add new flag "download_combined_frontend_spec_file" to frontend test execution file #15635

merged 2 commits into from Jun 29, 2022

Conversation

gp201
Copy link
Member

@gp201 gp201 commented Jun 26, 2022

Overview

  1. This PR fixes or fixes part of N/A.
  2. This PR does the following: A new flag download_combined_frontend_file is added to allow developers to download the combined-tests.spec.js to allow easier debugging.

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

image

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.

@gp201 gp201 requested a review from a team as a code owner June 26, 2022 02:47
@gp201 gp201 requested a review from DubeySandeep June 26, 2022 02:47
@oppiabot
Copy link

oppiabot bot commented Jun 26, 2022

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

@oppiabot oppiabot bot assigned gp201 Jun 26, 2022
@gp201 gp201 assigned DubeySandeep and unassigned gp201 Jun 26, 2022
@gp201
Copy link
Member Author

gp201 commented Jun 26, 2022

@DubeySandeep PTAL

@DubeySandeep
Copy link
Member

DubeySandeep commented Jun 27, 2022 •
edited

@gp201 Is it possible to get it reviewed from the frontend or testing team? (I've less experience around the changes in this PR, I'm fine with merging it once someone from the fronted/test team approves the PR)

@DubeySandeep DubeySandeep assigned gp201 and unassigned DubeySandeep Jun 27, 2022
DubeySandeep
DubeySandeep requested changes Jun 27, 2022
View reviewed changes
Copy link
Member

@DubeySandeep DubeySandeep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gp201 I took a quick pass and left few comments, PTAL!

scripts/run_frontend_tests.py Outdated
@@ -140,6 +145,11 @@ def main(args: Optional[Sequence[str]] = None) -> None:
# The value of `process.stdout` should not be None since we passed
# the `stdout=subprocess.PIPE` argument to `Popen`.
assert task.stdout is not None
# Prevents the wget command from running multiple times.
combined_test_spec_downloaded = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
combined_test_spec_downloaded = False
combined_test_spec_downloaded = False
Suggested change
combined_test_spec_downloaded = False
combined_spec_file_started_downloading = False

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

scripts/run_frontend_tests.py Outdated
if download_task:
# Wait for the wget command to download the combined-tests.spec.js
# file to complete.
download_task.wait()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any issue with calling wait just after running the wget command?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, moved the line.

scripts/run_frontend_tests.py Outdated
action='store_true'
)
_PARSER.add_argument(
'--download_combined_frontend_file',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it the spec file or the frontend file? (I see spec_file in variable names below)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec file. Changed the argument.

@gp201
Copy link
Member Author

gp201 commented Jun 28, 2022

@DubeySandeep PTAL

@gp201 gp201 assigned DubeySandeep and unassigned gp201 Jun 28, 2022
@oppiabot
Copy link

oppiabot bot commented Jun 28, 2022

Unassigning @DubeySandeep since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Jun 28, 2022
@oppiabot
Copy link

oppiabot bot commented Jun 28, 2022

Hi @gp201, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

@oppiabot oppiabot bot assigned gp201 Jun 28, 2022
@seanlip seanlip merged commit 06f9826 into oppia:develop Jun 29, 2022
@gp201 gp201 changed the title Add new flag "download_combined_frontend_file" to frontend test execution file Add new flag "download_combined_frontend_spec_file" to frontend test execution file Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DubeySandeep DubeySandeep DubeySandeep approved these changes

Assignees

@gp201 gp201

Labels
PR: LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gp201 @DubeySandeep @seanlip