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.

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

🏗 Follow up changes to integration tests on sauce labs #24623

Merged
merged 7 commits into from Sep 19, 2019
Merged

🏗 Follow up changes to integration tests on sauce labs #24623

merged 7 commits into from Sep 19, 2019

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Sep 18, 2019

Highlights:

  • Separate duplicate integration/saucelabs test status into integration/saucelabs-stable and integration/saucelabs-beta
  • Split up execution into stable and beta for push builds too
  • Report results / totals / errors for beta runs
  • Set an "error" status when zero tests are detected, but don't exit (allows recovery in cases like this)
  • Fix logging

Fixes this error:
image

Follow up to #24613

@rsimha
Copy link
Contributor Author

rsimha commented Sep 19, 2019

I was going to merge in a hurry to unblock master, until I realized that the test statuses are non-blocking. Will wait for a proper review tomorrow.

Copy link
Collaborator

@estherkim estherkim left a comment

Choose a reason for hiding this comment

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

Thanks for following up on my PR. I think it's cool to bubble up the test status of beta browsers, esp since these checks are non blocking.

build-system/tasks/runtime-test/helpers.js Outdated Show resolved Hide resolved
build-system/pr-check/remote-tests.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Thanks for the review! Feedback addressed.

build-system/tasks/runtime-test/helpers.js Outdated Show resolved Hide resolved
build-system/tasks/runtime-test/helpers.js Outdated Show resolved Hide resolved
build-system/pr-check/remote-tests.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Latest comment addressed. If you think there's a better way to do this, I'm happy to abandon this PR in favor of the one you started. (I mistakenly thought this was blocking master yesterday and tried putting together a quick fix.)

build-system/tasks/runtime-test/helpers.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@estherkim estherkim left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@rsimha rsimha merged commit ecebd34 into ampproject:master Sep 19, 2019
@rsimha rsimha deleted the 2019-09-18-SauceLabsReport branch September 19, 2019 21:40
@danielrozenberg
Copy link
Member

Eventually we wanted to make these test statuses blocking, so we'll have to make an exception in the test-status app to mark failing -beta runs as neutral instead of failed...

@rsimha
Copy link
Contributor Author

rsimha commented Sep 20, 2019

Eventually we wanted to make these test statuses blocking, so we'll have to make an exception in the test-status app to mark failing -beta runs as neutral instead of failed...

Good point. Github allows us to make a subset of the test status checks blocking. So we can make all but the -beta check blocking if we wanted, and leave it in the failed state for visibility. Whichever way we go, there is flexibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants