Amphtml: Java AMP Validator Syncing Business Logic with JS Changes & Travis Integration

[GeorgeLuo]

Make validator-java non-blocking for non Java Validator changes

Currently any validator file that is changed runs validator-java tests. However the Java engine may not be in sync with the changes being made to the JavaScript engine. As a result, those PRs may become blocked due to those tests.

Uncouple validator-java from non Java Validator files

In PR 27706, all validator related files caused validator-java to run. It would be beneficial to only run validator-java if there are files related to validator/java/... as resource file changes may cause test cases to fail.

The area of concern: this creates a state where contributors will clone the repository and find the validator fails in the build step (and is of course outdated).

[honeybadgerdontcare]

/cc @twifkak @rsimha

[honeybadgerdontcare]

A potential solution to keep testing protoascii changes over time for the Java validator is to fork the protoascii and proto now and then into validator/java/. This way if someone pulls down the Java code it'll work at that older version of the protoascii/proto without any failures that may exist due to this decoupling. The concated protoascii rules and validator.proto could be stored in validator/java/rules/ or equivalent.

[GeorgeLuo]

This is an elegant solution in delivering the point of unit tests. The argument against this would be the validator may be used in a misleading way, if anyone is building from source.

I suppose we could have some mechanism that tests the usefulness of the validator for the real resource files, and two sets of tests, but the main travis job will only block on the failure due to the mock tests.

I would say visibility that tests fail is still important to inform us that changes need to be made. This needs to reconcile with unblocking the main travis job from finishing.

[rsimha]

It certainly is possible to make it so that the java validator tests are no longer run for all validator-related changes in the amphtml repo, and run only when files in validator/java/ are edited. As you said, this can result in the java validator code being out of sync more often than not.

However, this will prevent us from running the java validator tests during push builds that are run for every master commit. Moreover, this will make it impossible to achieve item 2 in https://github.com/ampproject/amphtml/issues/27093#issuecomment-601348317 (auto-deployment of validator code to maven from amphtml push builds).

Let me know which way you'd like to deal with the testing problem, and if in the meantime, you'd like me to disable java validator tests for AMP code changes and for push builds.

[honeybadgerdontcare]

@rsimha For now lets disable the java validator tests for non java validator files. It's blocking PRs.

[GeorgeLuo]

agreed. Is it possible to condition item 2 (publish jar) on the attempt to run java validator tests? Basically, we would not try to publish if the tests were not ran.

[rsimha]

The discussion on deployments should probably be continued in #27093, but I'm responding here since we're all looking at this thread now 😃

Is it possible to condition item 2 (publish jar) on the attempt to run java validator tests? Basically, we would not try to publish if the tests were not ran.

The answer to your question is no, for a few reasons:

1. Only PR builds have knowledge of which files were affected. Based on this, they decide which tests to run.
2. Push builds run all tests for each commit to the master branch (and all our release branches). They have no notion of PRs, or which files were changed.
3. Now that there's no guarantee that the validator java tests are in sync with AMP's validator code, they are not expected to pass all the time as currently written. Therefore, we can no longer run them during push builds.
4. Deployment requires secure authentication tokens, which can only be done on push builds, not PR builds.
5. Finally, and most importantly, there is no precedent for deploying code to external repositories from amphtml push builds. We cannot afford to have unsuccessful deployments fail our Travis builds, which are critical to general development and releasing of the AMP library. Therefore, in order to deploy the java validator to maven, I'd recommend doing something similar to what is currently being done for other validator-related deployments to places like npm, by separately triggering a deployment out of band.

[GeorgeLuo]

Okay! All makes sense. Is it possible to run the test but produce a warning that the test failed? While there is no guarantee that test pass, we would like to address issues asap. Running the tests would go towards that.

Meanwhile we can discuss the dummy proto approach on this side and work on resolving the current conflict.

[rsimha]

Is it possible to run the test but produce a warning that the test failed?

This is done in #27788. You'll have to manually keep an eye on the Validator Tests job in our Travis builds to address any breakages.

[rsimha]

Update: With recent changes, we've arrived at a state where the java validator tests are not guaranteed to pass all the time on master.

With this in mind, here is what remains to be done:

• [ ] Fix #27939
• [ ] Find a means other than amphtml Travis builds to deploy the java validator (https://github.com/ampproject/amphtml/issues/27786#issuecomment-614264183)

Both these items are under the purview of @ampproject/wg-caching. Happy to help if you have any remaining questions.