Amphtml: Fix Java validator tests and merge maven upgrade PRs

Created on 22 Apr 2020  路  13Comments  路  Source: ampproject/amphtml

I'm filing this as a tracking issue based on the conversation starting at https://github.com/ampproject/amphtml/pull/27842#issuecomment-617553437.

There are two things to do:

  • [ ] Fix the Java validator test failures.
  • [x] Merge the backlog of package upgrade PRs for the Java validator that are all blocked by the test failures. (Done in #27951 and #28032)
Soon Flaky Tests Bug caching

All 13 comments

Assigning to @honeybadgerdontcare for triage

/cc @ampproject/wg-caching @GeorgeLuo

//cc @GeorgeLuo the first task is being handled in #27842.

I think we should do a manual update since there are so many outstanding but would like confirmation from @GeorgeLuo first.

That's a reasonable course, from a usability perspective the tests will fail already, changing the dependencies has a neutral-positive effect in the current state.

The changes to catch up the engine to yesterday's changes are somewhat expansive. I'm working on porting them over, but the whole of the changes this won't be resolved today. Can the upgrade PRs be forced through? Or are you guys waiting for a PR from my end to compile all of them?

I鈥檓 leery about force-merging all those dozen plus PRs because it involves using admin privileges. Would you be able to put them together into a single PR? I can help by force-merging it for you.

@GeorgeLuo As an aside, one of the larger commits in yesterday's changes is separated out as a formatting-only change. That may help with porting, as that specific commit can be ignored.

got it thanks 馃憤I was somewhat aware, but separating out by commit will be handy.

@GeorgeLuo Looks like we have a new batch of updates that came in today. I've currently labeled / assigned them to @ampproject/wg-caching folks, since we're still blocked from merging them normally.

I don't think these are as urgent as the previous batch (right now, I don't see any security vulnerabilities due to the new batch), so feel free to include them in your PR that fixes the tests once you've figured out how to do that.

PS: I acknowledge this is a glut of PRs to deal with, but once we're all caught up, the updates should be much less frequent.

Edit: Done in #28032

Ack. I started and scoped the changes yesterday. It'll be hard to estimate when they can go in, if significant unit test changes need to be implemented. So I may aggregate the dependency changes for a PR if those take an exorbitant amount of time.

@rsimha can you clarify the state of this job? As I understand it the completion of gulp validator-java is no longer a predicate to completing the job. So why did java come into play in the latest travis job? Was the logic added to watch for changes to under _validator/java_ path? thank you

As I understand it the completion of gulp validator-java is no longer a predicate to completing the job.

Successful completion of gulp validator-java is not a requirement for PRs that do not touch validator/java.

So why did java come into play in the latest travis job? Was the logic added to watch for changes to under _validator/java_ path?

Correct, it is a requirement for code that does touch that directory, to prevent _new_ breaking changes from being merged. Here's the logic (notice the presence / absence of OrDie):

https://github.com/ampproject/amphtml/blob/aca8b111c3a3e3d3133edfb2ffaa8acf38bed60c/build-system/pr-check/validator-tests.js#L77-L81

Ah awesome, thanks. The system works!

I think this has already been resolved last year. If I'm missing something, please reopen.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

mkhatib picture mkhatib  路  3Comments

radiovisual picture radiovisual  路  3Comments

Download picture Download  路  3Comments

torch2424 picture torch2424  路  3Comments

edhollinghurst picture edhollinghurst  路  3Comments