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

Attempt to fix Chrome on Travis #5382

Merged
merged 5 commits into from
Jul 28, 2018
Merged

Attempt to fix Chrome on Travis #5382

merged 5 commits into from
Jul 28, 2018

Conversation

apb7
Copy link
Contributor

@apb7 apb7 commented Jul 27, 2018 •
edited
Loading

Explanation

Changes:

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes.
  • The PR explanation includes the words "Fixes #bugnum: ...".
  • 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
Copy link

codecov-io commented Jul 27, 2018 •
edited
Loading

Codecov Report

Merging #5382 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5382   +/-   ##
========================================
  Coverage    47.73%   47.73%           
========================================
  Files          489      489           
  Lines        28005    28005           
  Branches      4327     4327           
========================================
  Hits         13368    13368           
  Misses       14637    14637

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 c1d63ba...3f0cf71. Read the comment docs.

@apb7 apb7 requested a review from seanlip July 27, 2018 23:57
seanlip
seanlip reviewed Jul 28, 2018
View reviewed changes
.travis.yml
@@ -69,6 +66,8 @@ before_install:
- export CHROME_BIN=/usr/bin/google-chrome-stable
- export DISPLAY=:99.0
- bash -e /etc/init.d/xvfb start
- wget --no-verbose -O /tmp/$(basename $CHROME_SOURCE_URL) $CHROME_SOURCE_URL
Copy link
Member

Choose a reason for hiding this comment

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

Just to check, what is CHROME_SOURCE_URL? Just wondering if it'll break in the future when Chrome updates again, and if it's possible to use something stable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have mentioned it in the PR description. I found a repository which stores deb files of various chrome versions: https://github.com/webnicer/chrome-downloads

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry. Where are these "repository settings"? Should we document the procedure for updating to a newer version of Chrome here, in comments?

(We will have to do that eventually. But better that happen under our control, rather than under our feet and breaking us every time.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case there is a problem with the link, we can directly change it by modifying this environment variable in Travis's repo settings.

Copy link
Member

Choose a reason for hiding this comment

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

I think, in this file, you should provide instructions on how exactly to do that (in comments). It's the natural place that people will look.

Maybe in a separate PR after this one, though, since we want to get this one in as quickly as possible to unbreak develop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here are the Travis repository settings: https://travis-ci.org/oppia/oppia/settings
The environment variables are under the same-named header.

Copy link
Member

Choose a reason for hiding this comment

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

OK, thanks! I've merged this PR. Can you put these instructions in a comment here, as mentioned previously?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will do this. Thanks!

seanlip
seanlip approved these changes Jul 28, 2018
View reviewed changes
Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @apb7! Feel free to merge whenever you like.

@apb7
Copy link
Contributor Author

apb7 commented Jul 28, 2018

Thanks, Sean!

@apb7
Copy link
Contributor Author

apb7 commented Jul 28, 2018

@seanlip: The build still seems to fail. I think we will have to go with the add-on provided by Travis.

@seanlip
Copy link
Member

seanlip commented Jul 28, 2018

It seems to pass for me?

@seanlip
Copy link
Member

seanlip commented Jul 28, 2018

Like, I'm looking here and it's mostly green

@apb7
Copy link
Contributor Author

apb7 commented Jul 28, 2018

Oh, I think it failed for the first try and then passed.

@apb7
Copy link
Contributor Author

apb7 commented Jul 28, 2018 •
edited
Loading

It failed here for the first two attempts.
Edit: This seems to be more of a protractor issue: angular/protractor#1283

@seanlip
Copy link
Member

seanlip commented Jul 28, 2018

Oh, that's interesting. Seems like a different error that may be related to waiting for Angular...

@apb7
Copy link
Contributor Author

apb7 commented Jul 28, 2018

I think we've had some previous similar problems: #4613 (comment)

@apb7
Copy link
Contributor Author

apb7 commented Jul 28, 2018

This test just timed out while trying to download Chrome: https://travis-ci.org/oppia/oppia/jobs/409159118#L518

@seanlip seanlip merged commit 4ae7e98 into oppia:develop Jul 28, 2018
@apb7 apb7 deleted the fix-chrome-on-travis branch July 28, 2018 01:06
@apb7 apb7 mentioned this pull request Jul 28, 2018
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanlip seanlip seanlip approved these changes

Assignees
No one assigned
Labels
PR: LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@apb7 @codecov-io @seanlip