Amphtml: Validation tests fails if output file does not have whitespaces in the copyright notice

Created on 24 Jan 2018  路  9Comments  路  Source: ampproject/amphtml

After some debugging while very confused, I have found an issue where if the copyright notice in the .out file of a validation test does not have the 2-whitespaces at lines 4,9 and 10, validation test fails. Note that .html part of the test does not need to have these whitespaces at all.

/to @honeybadgerdontcare
/cc @powdercloud

Soon Bug caching

Most helpful comment

another option to consider is always generating it if .out is missing, otherwise do the compare.

All 9 comments

This test is very simple. We take the .html file, prefix every line with "| " (pipe, space, space), and then insert lines with errors if we find them. The .out file shows the errors in context.

The problem is that this scheme is intended to be used with code that automatically regenerates the .out files from the .html files and have the user + reviewer inspect the diffs. It is not intended that the .out file is hand written.

I don't think we have ever provided that little binary in the github repo. That should probably be the issue that we fix.

I think we should fix this, many IDEs remove trailing spaces by default so even if this file is autogenerated, just opening it and saving can use tests to start failing which can be confusing.

I'm not strictly opposed to adding whitespace trimming to the automatic manipulation. However, there are lots of things that editors might do, such as trying to automatically format html, wrap strings at a certain number of columns, add newlines at ends of files, replace spaces with tabs or tabs with spaces.

The model is that the .out files aren't intended to be edited by hand. The .html files can be edited at will and then a script is run to regold the .out files, resulting in showing diffs when committing changes. The way to enforce that this was done is a test, like we have.

+1 to having a tool or option in validator/build.py that generates the *.out files.

another option to consider is always generating it if .out is missing, otherwise do the compare.

+1 for trimming whitespace or at least adding a Travis log output to this effect. This is like 3 hrs of life for everyone that writes another .out file. >.<

@aghassemi , is the auto-update feature enough to close this, or do you think we should debug the weird whitespace further?

@Gregable auto-update makes this mostly obsolete but auto-update does not seem to work: https://github.com/ampproject/amphtml/issues/14989 could you prioritize that issue?

Closing this per closing #14989.

Was this page helpful?
0 / 5 - 0 ratings