✨ Add cpp test of validator into pr checks

[antiphoton]

No description provided.

[amp-owners-bot]

Hey @rsimha! These files were changed:

.circleci/config.yml
.circleci/install_validator_dependencies.sh
build-system/pr-check/build-targets.js
build-system/pr-check/validator-tests.js

Hey @ampproject/wg-caching! These files were changed:

validator/.gitignore

[rsimha]

A couple initial comments. Happy to help in addressing them.

[rsimha]

Getting closer. A few more suggestions.

[antiphoton]

During the time the --update-tests are not available for C++, should we make this pr check as optional?

[rsimha]

During the time the --update-tests are not available for C++, should we make this pr check as optional?

We don't ever run --update-tests during CI. IIUC, It's only run during local development to update expected results when a developer (typically someone on @ampproject/wg-caching?) makes a change to the engine. (David or Greg can keep me honest here.)

So if these tests are stable and won't arbitrarily fail when runtime code changes, I'm in favor of checking them in as is.

[honeybadgerdontcare]

Raghu is correct. --update-tests is developer initiated and shouldn't be initiated elsewhere as that may have unintended consequences.

[rsimha]

Nice work in getting this to work @antiphoton! There's one more high-level point that needs to be addressed before this can be checked in.

I'm seeing that we've increased the running time of the Validator Tests job from ~5m to ~19m. I'm also seeing that the CPP tests take ~1 second to run, and that bulk of the time is spent on the build.

Unfortunately, this is going to have a non-trivial effect on CI. Restarting a flaky build will now result in a nearly 20 minutes of additional running time, not to mention the fact that we're now going to run this against almost every new AMP PR.

A couple questions:

• Is there any way to speed up the CPP build? Can we use parallelism? (For comparison, we build ~170 AMP components and ~300 ad vendor files with closure compiler in about 4 minutes.)
• Must the Validator CPP tests be triggered for all AMP changes? What if they are triggered only when validator code is changed?

/cc @ampproject/wg-caching

[honeybadgerdontcare]

• Must the Validator CPP tests be triggered for all AMP changes? What if they are triggered only when validator code is changed?

It can be triggered for validator only changes. I think this should cover it, @caoboxiao can you double check that these are the files/folders we'd want tested when changed by the C++ Validator.

validator/cpp/**
validator/testdata/**
validator/**.protoascii
validator/**.proto
extensions/**/validator-**.protoascii
extensions/**/validator-**.html
extensions/**/validator-**.out
extensions/**/validator-**.out.cpponly

[antiphoton]

validator/cpp/**
validator/testdata/**
validator/**.protoascii
validator/**.proto
extensions/**/validator-**.protoascii
extensions/**/validator-**.html
extensions/**/validator-**.out
extensions/**/validator-**.out.cpponly

Yes, this list covers all dependencies of c++ validator tests.

[antiphoton]

• Must the Validator CPP tests be triggered for all AMP changes? What if they are triggered only when validator code is changed?

I made some changes so that Validator Tests now skips amp validator-cpp when the PR does not change validator files.

[rsimha]

Thanks for patiently addressing all comments. With a ~7 minute total running time for all three validator test types, this PR now LGTM!

A future clean up item would be to make the bazel logs less noisy, and only print error messages. I'm okay with handling that via a separate PR.