🐛 Bug fix on java validator for failed test due to $DEFAULT at rule evaluation #27842

GeorgeLuo · 2020-04-17T21:16:07Z

business logic change + test for fix + update tagchowder to newer version

…version

amp-owners-bot · 2020-04-17T21:16:13Z

Hey @ampproject/wg-caching, these files were changed:

validator/java/WORKSPACE
validator/java/ampvalidator_checkstyle.xml
validator/java/build.sh
validator/java/pom.xml
validator/java/src/main/bin/copyValidatorJavaSource.sh
validator/java/src/main/java/dev/amp/validator/CdataMatcher.java
validator/java/src/main/java/dev/amp/validator/css/Canonicalizer.java
validator/java/src/main/java/dev/amp/validator/css/CssParsingConfig.java
validator/java/src/main/java/dev/amp/validator/parser/AMPHtmlParser.java
validator/java/src/main/java/dev/amp/validator/utils/CssSpecUtils.java
validator/java/src/main/java/dev/amp/validator/visitor/InvalidRuleVisitor.java
validator/java/src/main/proto/validator.proto
validator/java/src/test/java/dev/amp/validator/CdataMatcherTest.java
validator/java/src/test/java/dev/amp/validator/css/CanonicalizerTest.java
validator/java/src/test/java/dev/amp/validator/css/CssParsingConfigTest.java
validator/java/src/test/java/dev/amp/validator/utils/CssSpecUtilsTest.java

validator/java/pom.xml

rsimha · 2020-04-17T21:56:57Z

Tagging @ampproject/wg-caching, since it doesn't look like owners bot has the permission to @mention working groups

/cc @rcebulko @mrjoro

validator/java/src/main/java/dev/amp/validator/css/Canonicalizer.java

validator/java/ampvalidator_checkstyle.xml

twifkak · 2020-04-17T23:49:46Z

@Gregable This looks to be a port of cl/305733898 (#27784).

honeybadgerdontcare · 2020-04-17T23:54:49Z

@Gregable This looks to be a port of cl/305733898 (#27784).

Yes. They need to get these engine changes in so the tests can pass for Java.

rsimha · 2020-04-20T23:25:09Z

@GeorgeLuo @honeybadgerdontcare @Gregable A couple questions about these tests:

From the latest travis logs, it appears the tests are still broken. Let me know if this isn't expected, and if you need help debugging.
Several of the dependencies in validator/java/pom.xml are out of date, and some of them are showing up as security vulnerabilities in our repo (see 🏗📦 Switch renovate updates for amphtml from opt-in to opt-out #27692 (review)). I've enabled auto-upgrades for this file using our existing package auto-updater, but the java validator test failures are preventing us from merging all the upgrade PRs (15 and counting). Once the tests themselves are fixed, we'll need to a) re-test and re-run all these upgrade PRs or b) do a one-time manual upgrade of all packages in validator/java/pom.xml and then let automated maintenance take over. Can you help with this?

GeorgeLuo · 2020-04-21T01:56:34Z

@rsimha yes, I can run a clean pull and see if tests fail, due to discrepancies with my local repos. I might ask to enable debugging for the build if it works from my local environment, just for a little visibility.

rsimha · 2020-04-21T02:02:32Z

@rsimha yes, I can run a clean pull and see if tests fail, due to discrepancies with my local repos. I might ask to enable debugging for the build if it works from my local environment, just for a little visibility.

Thanks for the help. If you're developing on Linux or MacOS, you should be able to run these tests by checking out the amphtml repo and invoking gulp validator-java:

For local debugging, you'll see more logging by changing -verbose from 1 to 2 here:

amphtml/validator/java/BUILD

Line 74 in 62248e7

args=['-testjar','libamphtml_validator_test_lib.jar','-verbose','1'],

GeorgeLuo · 2020-04-21T02:23:17Z

Thank you! My maven build with a clean env was successful. When trying with gulp it actually looks it's hanging after the resource files are pulled down, with "Did you forget to signal async completion". It doesn't look like the tests are even ran.

This happens in the case when the files already exist as well. Either a resolution comes from the gulp job or our shell script to fetch resources needs modification. Have you seen this class of error before?

rsimha · 2020-04-21T02:33:07Z

Have you seen this class of error before?

I'm unfamiliar with this. Can you temporarily increase the verbosity in your PR and see what the Travis logs reveal?

GeorgeLuo · 2020-04-21T05:38:56Z

I can do that. here is what I'm observing

[01:23:34] Starting 'validator-java'...
install_bazel.sh Bazel binary detected at bazel-installer/bazel-3.0.0-installer-darwin-x86_64.sh; skipping installation
Both validator.proto and validator-all.protoascii exist
Copying Validator.java from /Users/gluo17/Documents/workspace/amphtml/validator/java/bazel-bin/amp/validator
done copying

I added the "done copying" to the end of amphtml/validator/java/src/main/bin/copyValidatorJavaSource.sh

It hangs after that. So it looks like it's not returning from that script gracefully, or whatever is calling that is not returning gracefully. What is the path from "gulp validator-java" to the shell script? I see it referenced in BUILD and nowhere else. But I'm not sure who is handling that BUILD file. It seems to be bazel.

GeorgeLuo · 2020-04-21T13:53:23Z

When I run the build and test script, the ./build.sh step takes 30 seconds or so, successfully though. Could there be some kind of timeout when bringing travis into the equation?

Curiously, the output here proceeds through to the test output, displaying failure. Has any of the build logic been changed since?

…m/GeorgeLuo/amphtml into GeorgeLuo-catchup-default-at-rule

change tagchowder

GeorgeLuo · 2020-04-21T20:12:22Z

It worked courtesy of @nhant01. bazel was silently failing during the build phase because we only updated the tagchowder library in maven and didn't do the same in the bazel WORKSPACE. we'll probably need to figure out how to splay these types of errors. I'm going to clean up some of the echos and such.

rsimha

Glad you were able to figure this out. Now that tests are green, I'm going to merge this in order to unblock all the java validator upgrade PRs.

rsimha · 2020-04-22T05:08:49Z

These tests are still failing during CI. See logs. Any idea what's going on?

/invite @GeorgeLuo @nhant01

amp-invite-bot · 2020-04-22T05:08:52Z

An invitation to join ampproject has been sent to @GeorgeLuo. I will update this thread when the invitation is accepted.

GeorgeLuo · 2020-04-22T15:51:40Z

unclear but pulling the latest proto seems to have introduced compilation failure cases to do with css specs. I imagine we're in a state where the .proto under java validator compiled fine (outdated), but the protoascii has introduced new models and flows. I'll get the engine changes in and then see about the tests.

rsimha · 2020-04-22T16:18:42Z

Thanks @GeorgeLuo. Meanwhile, we have a large backlog of package upgrade PRs for the Java validator that are all blocked by the test failures. Since validator/java/pom.xml currently has several outdated dependencies, would it make sense for you to do a one-time upgrade of all of them?

Under normal circumstances, once the file is up to date, there will be far fewer upgrade PRs each week, and they can be individually tested and merged as they appear.

Edit: I've filed #27939 to track this work.

remove at rule logic and associated test. Update tagchowder to newer …

c580ed0

…version

googlebot added the cla: yes label Apr 17, 2020

rsimha reviewed Apr 17, 2020

View reviewed changes

validator/java/pom.xml Show resolved Hide resolved

puppycrawl update

1a833c0

honeybadgerdontcare added Validator: Approved WG: caching labels Apr 17, 2020

update checkstyle version for puppycrawl

54054cb

rsimha reviewed Apr 17, 2020

View reviewed changes

validator/java/src/main/java/dev/amp/validator/css/Canonicalizer.java Outdated Show resolved Hide resolved

validator/java/ampvalidator_checkstyle.xml Outdated Show resolved Hide resolved

twifkak requested a review from Gregable April 17, 2020 23:48

GeorgeLuo added 2 commits April 17, 2020 21:39

Update Canonicalizer.java

736b3ca

Update ampvalidator_checkstyle.xml

bad24d9

Gregable approved these changes Apr 19, 2020

View reviewed changes

GeorgeLuo and others added 7 commits April 21, 2020 10:48

temporarily enable debug logging

af64252

Merge branch 'GeorgeLuo-catchup-default-at-rule' of https://github.co…

e2480b8

…m/GeorgeLuo/amphtml into GeorgeLuo-catchup-default-at-rule

Merge branch 'master' into GeorgeLuo-catchup-default-at-rule

3b6169a

Update validator.proto

3879030

Merge branch 'GeorgeLuo-catchup-default-at-rule' of https://github.co…

563a1ee

…m/GeorgeLuo/amphtml into GeorgeLuo-catchup-default-at-rule

remove block type references due to proto changes

9525261

some echos, change where ValidatorProtos is copied from

801a786

Update WORKSPACE

c3f75da

change tagchowder

GeorgeLuo added 5 commits April 21, 2020 16:14

Update BUILD

4106700

Update build.sh

8603bd2

Update build.sh

6891c10

Update CdataMatcherTest.java

efee723

Update CssParsingConfigTest.java

fac833c

rsimha approved these changes Apr 22, 2020

View reviewed changes

rsimha merged commit 3900c8d into ampproject:master Apr 22, 2020

rsimha mentioned this pull request Apr 22, 2020

Fix Java validator tests and merge maven upgrade PRs #27939

Closed

2 tasks

ampprojectbot added the PR use: In Beta / Experimental label Apr 28, 2020

ampprojectbot added PR use: In Stable PR use: In LTS labels May 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Bug fix on java validator for failed test due to $DEFAULT at rule evaluation #27842

🐛 Bug fix on java validator for failed test due to $DEFAULT at rule evaluation #27842

GeorgeLuo commented Apr 17, 2020

amp-owners-bot bot commented Apr 17, 2020 •

edited

rsimha commented Apr 17, 2020 •

edited

twifkak commented Apr 17, 2020

honeybadgerdontcare commented Apr 17, 2020

rsimha commented Apr 20, 2020 •

edited

GeorgeLuo commented Apr 21, 2020

rsimha commented Apr 21, 2020 •

edited

GeorgeLuo commented Apr 21, 2020

rsimha commented Apr 21, 2020 •

edited

GeorgeLuo commented Apr 21, 2020 •

edited

GeorgeLuo commented Apr 21, 2020 •

edited

GeorgeLuo commented Apr 21, 2020

rsimha left a comment •

edited

rsimha commented Apr 22, 2020

amp-invite-bot bot commented Apr 22, 2020

GeorgeLuo commented Apr 22, 2020

rsimha commented Apr 22, 2020 •

edited

🐛 Bug fix on java validator for failed test due to $DEFAULT at rule evaluation #27842

🐛 Bug fix on java validator for failed test due to $DEFAULT at rule evaluation #27842

Conversation

GeorgeLuo commented Apr 17, 2020

amp-owners-bot bot commented Apr 17, 2020 • edited

rsimha commented Apr 17, 2020 • edited

twifkak commented Apr 17, 2020

honeybadgerdontcare commented Apr 17, 2020

rsimha commented Apr 20, 2020 • edited

GeorgeLuo commented Apr 21, 2020

rsimha commented Apr 21, 2020 • edited

GeorgeLuo commented Apr 21, 2020

rsimha commented Apr 21, 2020 • edited

GeorgeLuo commented Apr 21, 2020 • edited

GeorgeLuo commented Apr 21, 2020 • edited

GeorgeLuo commented Apr 21, 2020

rsimha left a comment • edited

Choose a reason for hiding this comment

rsimha commented Apr 22, 2020

amp-invite-bot bot commented Apr 22, 2020

GeorgeLuo commented Apr 22, 2020

rsimha commented Apr 22, 2020 • edited

amp-owners-bot bot commented Apr 17, 2020 •

edited

rsimha commented Apr 17, 2020 •

edited

rsimha commented Apr 20, 2020 •

edited

rsimha commented Apr 21, 2020 •

edited

rsimha commented Apr 21, 2020 •

edited

GeorgeLuo commented Apr 21, 2020 •

edited

GeorgeLuo commented Apr 21, 2020 •

edited

rsimha left a comment •

edited

rsimha commented Apr 22, 2020 •

edited