Skip to content

♻️ Modernize <amp-audio> tests#35650

Merged
alanorozco merged 2 commits intoampproject:mainfrom
alanorozco:amp-audio-test
Aug 17, 2021
Merged

♻️ Modernize <amp-audio> tests#35650
alanorozco merged 2 commits intoampproject:mainfrom
alanorozco:amp-audio-test

Conversation

@alanorozco
Copy link
Copy Markdown
Member

  • async/await
  • More simply, use HTML templates for element creation.
  • Style nits here and there.

@alanorozco alanorozco requested review from caroqliu and dmanek August 12, 2021 20:29
@alanorozco alanorozco marked this pull request as ready for review August 12, 2021 20:30
# Conflicts:
#	extensions/amp-audio/0.1/test/test-amp-audio.js
@alanorozco alanorozco merged commit 48bc93d into ampproject:main Aug 17, 2021
@alanorozco alanorozco deleted the amp-audio-test branch August 17, 2021 16:42
@alanorozco alanorozco mentioned this pull request Aug 26, 2021
8 tasks
AnuragVasanwala added a commit to rtCamp/amphtml that referenced this pull request Aug 29, 2021
Recently updated Classic `amp-audio` tests to remove this odd creation function and use HTML templates instead on ampproject#35650

Co-Authored-By: Alan Orozco <alanorozco@users.noreply.github.com>
dethstrobe pushed a commit that referenced this pull request Apr 12, 2022
* Initiate amp-audio bento component

* Make Audio component functional

* ♻️ Initial Test Component

* 🧪 Experimental Test Case Implementation

* 🧪 Refactoring

* ♻️ Comments and Prettify

* 🐛 Minor fixes

* ♻️ Minor fix for test case

* ♻️ `propagateAttributes` moved to `amp-audio.js`

* 🚮 Removed duplicate entry for `amp-date-display` in `bundles.config.extensions.json`

* ♻️ `isStoryDescendent` is moved to isolated AMP layer

* ♻️ `toggleFallback` refactored into `onLoad` and `onError`

`toggleFallback` can be on `onLoad` / `onError` / etc. instead. The Preact layer does not care what logic is passed here, only when it should be called.

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* 🚮 Removed unnecessary `toggleFallback` from `component.js`

* 🚮 Removed unnecessary comments

* 🐛 Fixed support to load from `<source>`

Added support to load audio from multiple `<source>` tags.

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* 🚮 Removed unnecessary commented code-block

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* ♻️ Initial value updated for `audioRef`

`useRef` should be initialised with `null`.

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* ♻️ `props` destructured into inline

* 🚮 Removed unused prop `children` from dependency

* ♻️ `<ContainWrapper>` added

* 🐛 Fixed bug related to `ARIA` attributes

* ♻️ Type for `ARIA` attributes added

* ♻️ Type definition are sorted for readability

* ♻️ Test-case correction

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* ♻️ Fix for `ARIA` attributes

* ✅ 🐛 Fix for `should load audio through sources`

This will not be defined once we have sources prop defined in `base-element.js`. Instead assert `expect(audio.childNodes).to.have.lengthOf(2)`since the third child is disregarded due to being not a `<source>` tag.

* ✅ ♻️ `element` instead of `audio`

By using `<ContainWrapper>`, `offsetWidth` and `offsetHeight` should be
evaluated on `element` instead of child `<audio>`.

* 🚮 `propagateAttributes` removed

`propagateAttributes` is passing attributes from `<amp-audio>`
to the `<audio>` element it created.

The Bento version does not have to use this utility because
the `BaseElement['props']` definition already takes care of this.

* ✅ ♻️ Fix for API function `play` and `pause`

Play-Pause should not work when `amp-audio`
is a direct decendent of `amp-story`.

* ✅ ♻️ Refactored `amp-audio` import

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* ♻️ 🚮 Removed unnecessary comment for test

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* ✅ 🚮 Removed unnecessary import

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* ♻️ Removed unnecessary `isPlaying` dependency

* ♻️ `triggerAnalyticsEvent` moved to AMP layer

* ✅ ♻️ `getAmpAudio` improvised for `1.0` test suite

This helper was taken from the `0.1` test suite and is improvised for `1.0` test suite.

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* ♻️ Fixed test case import

* 🏗 Updated validation rules

* ✅ Added preact component test-cases

* ♻️ `lint` and `prettify`

* ♻️ `listen` helper replaced by `onPlaying`

`listen` helper is imperative way of attaching event handlers.
We can instead use the `onPlaying` prop.

* 📖 Markdown YAML updated for experimental `bento`

* 🐛 Corrected bad type error

* ✅ ♻️ Simplified selector for `audio`

* ✅ ⏪ Uncommented failed test-cases for review

* 🏗 Updated validation rules

Also, copyright year updated to 2021 for validator files

* ♻️ Added default media `METADATA`

* ♻️ Added default value for `autoplay, loop, muted`

* ♻️ Minor code fixes

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* ♻️ Simplify `playCallback` and `pauseCallback`

To simplify object name, rename:
`playCallback` -> `play`
`pauseCallback` -> `pause`

* ♻️ `...rest` should be attached to `ContainWrapper`

`rest` should be attached to `ContainWrapper` so defined styles dimensions can be applied to the container.

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* 🐛 `realWin` required instead of `sandboxed` for Preact Test

For the `should load audio through sources`, because we are measuring the component, it needs to be attached to the DOM. In order to attach it, we need to run the test under a `realWin` and access DOM via the given env.

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* 🐛 Need to attach mounted component to the document

It is needed to have the mounted component attach to the document by providing the `attachTo` option.

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* 🐛 Use `.getDOMNode()` for `.offsetWidth`

* 🐛 Fixes for failing `audio` props

These were failing because the component actually renders an outer `ContainWrapper`, which was not given any of these props. So we have to actually get the `<audio>` instance to assert the props we want.

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* 🏗 ⏪ Bento components do not support `AMP4HTML`

Bento components are only for `html_format: AMP` for now.
They might support `html_format: AMP4HTML` in future!

* ♻️ Fixes for `style`

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* ♻️ 🚮 Removed unnecessary `unlayout` block

* ♻️ Added interface `AudioDef.AudioApi` API Action

* ♻️ Propagate `<source>`  children for preact

`<source>` will only work if we also modify
`component.js` to propagate children.

* ♻️ Cleanup, lint and prettify

* 🐛 Fixed preact test case error

* ♻️ Formatting and minor fixes

Co-authored-by: Alan Orozco <alanorozco@users.noreply.github.com>

* ♻️ `audioPlaying` renamed to `onPlaying`

With `metadata` name fixes and prettify.

* 📖 ⏪ `ads` added back to YAML

* ♻️ 🚮 `isInvocationValid` removed from the Bento implementation

Reference: #35643

* ♻️ Cleanup, lint and prettify

* 🚮 `amp-story` descendant check TestCase removed

Refer: #35643

* ♻️ Cleanup, lint and prettify

* ✨ Added support for `loading` attribute

By default, `loading` is set to `lazy`.

* ♻️ `source` is concidered instead of `children`

Let's use `sources` or `children`, but not both.
`sources` would be consistent with the video players.

* ♻️ Formatted html with `<!-- prettier-ignore -->`

Formatted with ` <!-- prettier-ignore -->` right above the opening
`<head>` tag. This will prevent the boilerplate code from formatting.

* ⏪ Removed `validateMediaMetadata` for component

 The validation step for video components on Bento are excluded for now.

* 🏗 Add a new `bento_supported_version` tag

Remove `deprecated_allow_duplicates: true` and `requires_usage: EXEMPTED` in new `1.0` versions.

Use new `bento_supported_version` tag.

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* ♻️ Improved type definition

Use `?SampleTypeDef` instead of `!SampleTypeDef | null`

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* ♻️ Improved event handler attachment

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* 🚮 Removed unnecessary comments

* ♻️ Safer way to trigger `onPlaying` and `onPause` handlers

Contributor: @alanorozco

A safer way to trigger `onPlay` and `onPause` props is to call them from event handlers like audioPlaying. Please move into handlers.

The idea is right, except for one small detail. play and playing are different events on the HTMLMediaElement spec, so let's rename the onPlay prop to onPlaying.

* 🏗 Fixes for validation

Co-authored-by: Alan Orozco <alanorozco@users.noreply.github.com>

nit: This quirky formatting is another inherited property, we could format this more nicely:

1. Temporarily, add `<!-- prettier-ignore -->` right above the opening `<head>` tag. This will prevent the boilerplate code from formatting.
2. Format with prettier locally, or on `prettier.io/playground/` (choose html parser on the options on the left side).
3. Remove `<!-- prettier-ignore -->`

* ♻️ Extra space removed for validation file

* 🚮 Removed copyright header from all `*.js` files

Co-Authored-By: Alan Orozco <alanorozco@users.noreply.github.com>
Co-Authored-By: honeybadgerdontcare <1340565+honeybadgerdontcare@users.noreply.github.com>

* 🚮 Removed copyright header from `protoascii`

Co-Authored-By: Alan Orozco <alanorozco@users.noreply.github.com>

* ♻️ Removed `loading` from `BaseElement`

We should not expose control of `loading` at this level—let's remove the attribute. (The prop on `component.js` should stay as-is.)

Co-authored-by: Alan Orozco <alanorozco@users.noreply.github.com>

* ♻️ Removed `EMPTY_METADATA` for `artist` and `artwork`

`EMPTY_METADATA.artwork` is an Array, which does not make sense as a string.

Let's set it to an empty string instead. It's okay to remove the default for `artwork` and and `artist`.

Co-authored-by: Alan Orozco <alanorozco@users.noreply.github.com>

* ♻️ Removed `default` for `loop` and `muted` from `BaseElement`

Remove `default` from booleans at this level, set on component props instead.

Co-authored-by: Alan Orozco <alanorozco@users.noreply.github.com>

* ♻️ Removed `autoplay` for now, `useInOb` will be used in future

To be clear - does `amp-audio:0.1` begin playing immediately once it enters viewport if given autoplay? If so, I think we should consider using the useInOb hook @dmanek is working on to do this feature, perhaps in a separate PR.

Having it `autoplay` immediately is not a good UX, and short of (potentially) implementing the viewport based logic, should be removed from the AMP layer for now.

Co-Authored-By: Alan Orozco <alanorozco@users.noreply.github.com>
Co-Authored-By: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* 🚮 Removed `EMPTY_METADATA`

Co-Authored-By: Alan Orozco <alanorozco@users.noreply.github.com>
Co-Authored-By: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* ♻️ Minor fixes for TestCase

Co-authored-by: Alan Orozco <alanorozco@users.noreply.github.com>

* 🚮 Removed copyright header from validator, ♻️Re-created validator

Co-Authored-By: Alan Orozco <alanorozco@users.noreply.github.com>

* ♻️ Fix to trigger `element.toggleFallback` for `onError`

Co-Authored-By: Alan Orozco <alanorozco@users.noreply.github.com>

* ♻️ Fixes for TestCase

Recently updated Classic `amp-audio` tests to remove this odd creation function and use HTML templates instead on #35650

Co-Authored-By: Alan Orozco <alanorozco@users.noreply.github.com>

* Latest updates to

* ♻️ Removed unnecessary var `ampAudio`

* ♻️ `lint` and `prettify`

* 🏗 Switched to latest LTS version of node

Node version v14.17.5 (latest LTS)
python version 2.7.16
npm version 6.14.14

* ♻️ Fix for preact component TestCase

* Fix bento amp-audio unit test

* 🧪 Experimental test for `done() called multiple times in hook`

Two consecutive test run resolves this error on local machine. As I do not have capability to restart CircleCI, I need to push this commit just to trigger re-run CircleCI to check.

* 🏗 Updated validation rules

Running `amp validator --update_tests` have updated `package-lock.json` files.

Co-Authored-By: honeybadgerdontcare <1340565+honeybadgerdontcare@users.noreply.github.com>

* ♻️ Fix import alias

* ♻️ Update derived class extends using `setSuperClass`

* Fix test case and build issue

* Remove addon-knobs dependency from storybook files

* Update bundles config with new properties

* 🖍 Add CSS (as per #32180)

* ♻️ Prefix `Bento` to Preact Component (`BentoAudio`)

Co-authored-by: Edi Amin <to.ediamin@gmail.com>
Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
Co-authored-by: Alan Orozco <alanorozco@users.noreply.github.com>
Co-authored-by: honeybadgerdontcare <1340565+honeybadgerdontcare@users.noreply.github.com>
Co-authored-by: Deepak Lalwani <deepak.lalwani81@gmail.com>
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.

4 participants