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

🐛 amp-ad type blade - fix bladeOnLoad callback #26627

Merged
merged 4 commits into from
Feb 26, 2020
Merged

🐛 amp-ad type blade - fix bladeOnLoad callback #26627

merged 4 commits into from
Feb 26, 2020

Conversation

gershman
Copy link
Contributor

@gershman gershman commented Feb 5, 2020

  • fix function: createContainer - bug: global was undefined fix: move function to be under the main export function

@amp-owners-bot amp-owners-bot bot requested a review from lannka February 5, 2020 09:06
@gershman
Copy link
Contributor Author

gershman commented Feb 9, 2020

@lannka Hi I see that the build didn't pass, I don't think it something related to my pull request. Can you help?

@lannka
Copy link
Contributor

lannka commented Feb 10, 2020

The test failure seems to be a flake. Restarted.

@gershman
Copy link
Contributor Author

@lannka hi when will this be merge and deployed ?

@gershman
Copy link
Contributor Author

@mrjoro @lannka Hi any updates when this will be merged and released, it is urgent for us. thx.

@gershman
Copy link
Contributor Author

@lannka ?

@yonida
Copy link

yonida commented Feb 23, 2020

@lannka @gmajoulet @erwinmombay
any update on this?

@gmajoulet
Copy link
Contributor

I restarted the failing tests, if they still don't pass, could you please sync your PR with the master branch?

@gershman
Copy link
Contributor Author

gershman commented Feb 24, 2020 •
edited
Loading

@lannka @gmajoulet @erwinmombay - Hi, I merged from master and rerun the tests, they still failed. So, I run gulp pr-check vs master branch and the same test fails there.

pr-check.js: Commit log since branch master was forked from master at 82cce82:
* 82cce8211 Matt Mower - (HEAD -> master, upstream/master) custom-element: Minor test improvements (#26923) (10 hours ago)

@gmajoulet
Copy link
Contributor

I restarted them ~5 times but they keep failing for different reasons. :(

Can someone from @ampproject/wg-infra chime in?

@rsimha
Copy link
Contributor

rsimha commented Feb 26, 2020

I restarted them ~5 times but they keep failing for different reasons. :(

Can someone from @ampproject/wg-infra chime in?

Do you have failure logs, or remember which tests failed? (Restarted build means I can't see the old ones.)

/cc @zhouyx who is build cop this week, and can help with skipping any obviously flaky tests.
/cc @rcebulko who is working on stabilizing our Sauce Labs infrastructure, in case the failures were due to VM disconnections.

@zhouyx
Copy link
Contributor

zhouyx commented Feb 26, 2020

Looks like all test passed. Happy to skip tests if flakiness continues.

@rcebulko
Copy link
Contributor

If this occurs again, could you stick the logs in a Gist and comment? That will help us debug. There is a known issue where we're hitting Sauce Labs concurrency limits, which may have been the culprit here.

@gmajoulet
Copy link
Contributor

Looks like cc'ing the infra people scared the flaky tests away, thanks all :))

@gmajoulet gmajoulet merged commit e740348 into ampproject:master Feb 26, 2020
@gershman gershman deleted the amp-ad--blade-fix-create-container-func branch February 27, 2020 07:24
robinvanopstal added a commit to jungvonmatt/amphtml that referenced this pull request Feb 27, 2020
@robinvanopstal
Merge branch 'master' into component-intro-tables
4d2af31 
* master: (54 commits)
  inabox-resources: Minor test improvement (ampproject#26916)
  DocInfo: replace metaTags with viewport in API (ampproject#26687)
  🐛 SwG now uses AMP sendBeacon interface (ampproject#26970)
  🏗 Allow array destructuring on preact hooks (ampproject#26901)
  Gulp Dep Check: fail on unused entries (ampproject#26981)
  Update no-import lint rule to forbid sub-paths (ampproject#26531)
  🐛 amp-ad type blade - fix bladeOnLoad callback (ampproject#26627)
  📖 Clarify when max-age is required (ampproject#26956)
  ♻️ Consolidate players as .i-amphtml-media-component (ampproject#26967)
  Add Preact Enzyme tests (ampproject#26529)
  Fixes `update_tests` flag on `gulp validator` (ampproject#26965)
  📦 Update dependency google-closure-library to v20200224 (ampproject#26986)
  🏗 Transform aliased configured components (ampproject#26541)
  ✨ InaboxResources: Observe intersections for some elements' viewportCallbacks (ampproject#26942)
  variable substitutions: Support allowlist lookup in AmpDocShadow (ampproject#26731)
  cl/297197875 Revision bump for ampproject#26877 (ampproject#26982)
  Json fix (ampproject#26971)
  📦 Update dependency mocha to v7.1.0 (ampproject#26976)
  Add documentation for amp-access-scroll (ampproject#26782)
  make controls always shown in amp for email (ampproject#25714)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lannka lannka lannka approved these changes

Assignees
No one assigned
Labels
cla: yes PR use: In Beta / Experimental PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@gershman @lannka @yonida @gmajoulet @rsimha @zhouyx @rcebulko @googlebot @ampprojectbot