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

Add unified sanitizer smoke test #4549

Open
schlessera opened this issue Apr 8, 2020 · 3 comments
Open

Add unified sanitizer smoke test #4549

schlessera opened this issue Apr 8, 2020 · 3 comments
Labels
P2 Low priority Testing Issues related with Unit, E2E, Smoke, and other testing requirements/needs WS:Perf Work stream for Metrics, Performance and Optimizer

Comments

@schlessera
Copy link
Collaborator

Feature description

I think it would be valuable to have a single test that runs a big chunk of HTML through the sanitizer which contains at least 1 validation error for each of the different sanitizer.

This would allow us to ensure that all sanitizers are active and working.

As an example of what this would catch early on, see #4535 (review), which demonstrates that:

  • I'm a fool;
  • PRs that have passing tests can still fail at a fundamental level, which would only be caught during QA right now.

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • Have at least 1 test that would fail if any single one of the sanitizers is inactive or ineffective.

Implementation brief

QA testing instructions

Demo

Changelog entry

@schlessera schlessera added the Infrastructure Changes impacting testing infrastructure or build tooling label Apr 8, 2020
@westonruter
Copy link
Member

westonruter commented Apr 8, 2020

What about running the AMP Validator CLI tool (the amphtml-validator) on $content after checking assertEqualMarkup:

$sanitizer->sanitize();
$content = $dom->saveHTML( $dom->documentElement );
$this->assertEqualMarkup( $expected, $content );

This would slow down the tests quite a bit most likely, so maybe that should be done behind a special flag.

@schlessera
Copy link
Collaborator Author

It would only need to be done once, not per PHP version, so it could be a separate, parallel testing step.

@westonruter
Copy link
Member

Agreed. Similar to the external-http test.

@amedina amedina added Testing Issues related with Unit, E2E, Smoke, and other testing requirements/needs and removed Infrastructure Changes impacting testing infrastructure or build tooling labels Apr 10, 2020
@westonruter westonruter added the P2 Low priority label Apr 12, 2020
@kmyram kmyram added the WS:Perf Work stream for Metrics, Performance and Optimizer label Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Low priority Testing Issues related with Unit, E2E, Smoke, and other testing requirements/needs WS:Perf Work stream for Metrics, Performance and Optimizer
Projects
Archived in project
Development

No branches or pull requests

4 participants