Skip to content

build: cancel screenshot diff if there is no activity #6957

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.

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

Merged

Conversation

devversion
Copy link
Member

@devversion devversion commented Sep 9, 2017

  • No longer just gives the screenshot diff task 8 minutes to pass, because the run time can vary. Now it just exits the process if the last action in the task takes longer than 6 minutes.
  • Instead of just closing the firebase connection, the process will be exited with an error code. Just closing the connection is not helpful and breaks the screenshot diff Web Interface.

Note: Should wait for #6956 before review.

@devversion devversion requested a review from jelbourn as a code owner September 9, 2017 11:53
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 9, 2017
@devversion devversion added the blocked This issue is blocked by some external factor, such as a prerequisite PR label Sep 9, 2017
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -34,6 +34,9 @@ const FIREBASE_IMAGE = `${TEMP_FOLDER}/screenshot/images`;
const FIREBASE_DATA_GOLDENS = `screenshot/goldens`;
const FIREBASE_STORAGE_GOLDENS = 'goldens';

const lastActionTimeout = 1000 * 60 * 6;
const lastActionRefreshInterval = 1000 * 45;
Copy link
Member

Choose a reason for hiding this comment

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

Add comments with the human-readable times?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Done

* No longer just gives the screenshot diff task 8 minutes to pass, because the run time can vary. Now it just exits the process if the last action in the task takes longer than 6 minutes.
* Instead of just closing the firebase connection, the process will be exited with an error code. Just closing the connection is not helpful and breaks the screenshot diff Web Interface.
@devversion devversion force-pushed the build/fix-e2e-task-screenshot-exit branch 2 times, most recently from 3ab54b7 to e9d90e0 Compare September 12, 2017 10:19
@devversion devversion force-pushed the build/fix-e2e-task-screenshot-exit branch from e9d90e0 to 1f10516 Compare September 12, 2017 11:42
@jelbourn jelbourn added action: merge The PR is ready for merge by the caretaker pr: merge safe and removed blocked This issue is blocked by some external factor, such as a prerequisite PR labels Sep 13, 2017
@mmalerba mmalerba merged commit 34b5620 into angular:master Sep 15, 2017
josephperrott pushed a commit to josephperrott/components that referenced this pull request Sep 15, 2017
* build: cancel screenshot diff if there is no activity

* No longer just gives the screenshot diff task 8 minutes to pass, because the run time can vary. Now it just exits the process if the last action in the task takes longer than 6 minutes.
* Instead of just closing the firebase connection, the process will be exited with an error code. Just closing the connection is not helpful and breaks the screenshot diff Web Interface.

* Add comments
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants