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

Fix test flake #10083

Merged
merged 6 commits into from
Jun 22, 2017
Merged

Fix test flake #10083

merged 6 commits into from
Jun 22, 2017

Conversation

jridgewell
Copy link
Contributor

@jridgewell jridgewell commented Jun 21, 2017

Fixes #10053.

@@ -30,7 +30,7 @@ if (!window.validatorLoad) {
})();
}

describe.configure().retryOnSaucelabs().run('example', function() {
describe.skip('example', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why skip all of these? Can we fix the failing example to validate or remove it from the list? If flaky on SauceLabs, I think a .skipSauceLabs() would be great to have especially for tests like this that don't really need cross-browser testing. ( as far as I remember, we run all tests including integration on Travis using latest Chrome in addition to running integration on SauceLabs, @rsimha-amp is still still the case > )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we skip sauce labs, when will these ever be run?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just verified that gulp test (which runs on Travis with latest Chrome as part of the pre-submit and unit test job) does indeed run integration tests. (it runs test/**/*)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@rsimha rsimha Jun 22, 2017

Choose a reason for hiding this comment

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

@aghassemi, here is where the unit tests are run for PRs and on master. We used to run using old chrome on saucelabs, but no longer do.

I didn't think we ran integration tests with gulp test, unless the --integration arg was passed in, so your comment above comes as a surprise to me. (See this.)

Copy link
Contributor

Choose a reason for hiding this comment

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

gulp test uses testPaths which is test/**/*. We definitely want to keep running integration tests on Travis with latest Chrome, so maybe the job can be renamed from presubmit and unit tests to something like presubmit and all tests on latest Chrome and the integration tests job can become integrations tests on SauceLabs

Copy link
Contributor

Choose a reason for hiding this comment

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

@aghassemi, fair enough. I'll review pr-check.js and do some clean up.

@jridgewell
Copy link
Contributor Author

@aghassemi approval? 😛

@jridgewell jridgewell merged commit 623c45e into ampproject:master Jun 22, 2017
@jridgewell jridgewell deleted the flake branch June 22, 2017 17:24
jridgewell added a commit that referenced this pull request Jun 22, 2017
* Remove video integration iframes

* Skip flakey development errors integration tests

* Only run vis-state tests on chrome

* Skip more flakey development integration tests

* fix

* skipSauceLabs
jridgewell added a commit that referenced this pull request Jun 22, 2017
* Remove video integration iframes

* Skip flakey development errors integration tests

* Only run vis-state tests on chrome

* Skip more flakey development integration tests

* fix

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

Successfully merging this pull request may close these issues.

None yet

4 participants