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 a way to validate all HTML test fixtures, even skipped ones #34000

Merged
merged 1 commit into from
Apr 24, 2021
Merged

🏗 Add a way to validate all HTML test fixtures, even skipped ones #34000

merged 1 commit into from
Apr 24, 2021

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Apr 24, 2021

This PR adds a way to validate HTML fixtures that are currently skipped from checks due to existing errors.

Usage:

# Step 1a
amp validate-html-fixtures # Passes because fixtures with errors are skipped

# Step 1b
amp validate-html-fixtures --include_skipped # Fails with a huge number of errors

# Step 2
# Pick a file with validation errors from step 1b and fix it
# Optional: Add the amphtml-validator extension to VSCode for easy detection of validation errors

# Step 3
amp validate-html-fixtures --include_skipped --local_changes # Shows if the file just edited is indeed fixed

# Step 4
# Once fixed, remove the file from the allowlist in htmlFixtureGlobs in build-system/test-configs/config.js

# Repeat steps 2-4 with as many files as you'd like, and send out a PR

With this, it will become significantly easier for us to fix the huge list of validation errors in our test fixtures. See #25145 (review) and #25149 (comment) for why this is important.

Follow up to #33997
Partial fix for #25149

@rsimha rsimha self-assigned this Apr 24, 2021
Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

Is include_skipped even needed for local_changes? I'd imagine if you modified one of the fixtures, then you can take it all the way and make it valid, too.

@rsimha
Copy link
Contributor Author

rsimha commented Apr 24, 2021

Is include_skipped even needed for local_changes? I'd imagine if you modified one of the fixtures, then you can take it all the way and make it valid, too.

Yep, --include_skipped is only needed with --local_changes if you haven't yet removed the fixture from the allowlist. In practice though, it's useful to first check if the fixture is valid before removing it from the allowlist.

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.

None yet

3 participants