ampproject / amp-toolbox

A collection of AMP tools making it easier to publish and host AMP pages.
Apache License 2.0
450 stars 242 forks source link

follow up to the addition of AmpStoryCssTransformer #1271

Closed erwinmombay closed 2 years ago

erwinmombay commented 2 years ago
erwinmombay commented 2 years ago

@sebastianbenz could you give some guidance on how to write an end to end test for this. I want to make sure the RewriteAmpUrls transformer doesn't conflict with how i'm writing the code here and doesn't do something like "lts/lts" in its path

sebastianbenz commented 2 years ago

You can add end-to-end tests via these steps:

  1. Create a new directory with a story input file here: https://github.com/ampproject/amp-toolbox/tree/main/packages/optimizer/spec/end-to-end
  2. Then run npm run test:optimizer:snapshot to generate the expected output
  3. Manually inspect the output for any problems
erwinmombay commented 2 years ago

@sebastianbenz is there a way to similarly turn on an experiment flag in end-to-end tests like how i did it with lts as a comment or through a config.json?

sebastianbenz commented 2 years ago

e2e tests automatically run for default, lts and paired.

sebastianbenz commented 2 years ago

(paired doesn't make sense in the story sense, so it's safe to ignore)

erwinmombay commented 2 years ago

@sebastianbenz would it make sense for me to create a "experimental" spec where i turn on experimental flags?

sebastianbenz commented 2 years ago

How about creating a story specific configuration here: https://github.com/ampproject/amp-toolbox/blob/main/packages/optimizer/spec/end-to-end/EndToEndSpec.js#L33

erwinmombay commented 2 years ago

@sebastianbenz PTAL. (tests are failing because main is broken, needs https://github.com/ampproject/amp-toolbox/pull/1272 merged)

erwinmombay commented 2 years ago

@sebastianbenz tests are green now. Please take another look. Thanks!

sebastianbenz commented 2 years ago

LGTM, one suggestion

erwinmombay commented 2 years ago

committed suggestion. should be green now and mergeable

sebastianbenz commented 2 years ago

Thanks Erwin!