Skip to content

[BEAM-11436] Never run checkerframework on tests - #14101

Merged
TheNeuralBit merged 3 commits into
apache:masterfrom
TheNeuralBit:BEAM-11436
Mar 3, 2021
Merged

[BEAM-11436] Never run checkerframework on tests#14101
TheNeuralBit merged 3 commits into
apache:masterfrom
TheNeuralBit:BEAM-11436

Conversation

@TheNeuralBit

Copy link
Copy Markdown
Member

Post-Commit Tests Status (on master branch)

LangSDKDataflowFlinkSamzaSparkTwister2
GoBuild Status---Build Status---Build Status---
JavaBuild StatusBuild Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build StatusBuild Status
Build Status
Build Status
Build Status
PythonBuild Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
---Build Status---
XLangBuild StatusBuild StatusBuild Status---Build Status---

Pre-Commit Tests Status (on master branch)

---JavaPythonGoWebsiteWhitespaceTypescript
Non-portableBuild StatusBuild Status
Build Status
Build Status
Build Status
Build StatusBuild StatusBuild StatusBuild Status
Portable---Build Status------------

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

Note this doesn't remove the suppressions added in 5675108. I thought I might be able to back it out with git revert restricted to just test directories, but for some reason git flags trivial merge conflicts for seemingly any test file that's had a change made to it in the meantime, even though the changes are rarely on the same lines.

Maybe I can come up with a better approach in the morning.. (generate a patch and reverse it?)

@codecov

codecovBot commented Feb 26, 2021

Copy link
Copy Markdown

Codecov Report

Merging #14101 (c159f45) into master (7a27c79) will increase coverage by 0.37%.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## master #14101 +/- ##
==========================================
+ Coverage 83.04% 83.42% +0.37% 
==========================================
Files 469 469 Lines 58465 60259 +1794 ==========================================
+ Hits 48555 50271 +1716 - Misses 9910 9988 +78 
Impacted FilesCoverage Δ
...sdks/python/apache_beam/internal/metrics/metric.py86.45% <0.00%> (-1.05%)⬇️
...apache_beam/portability/api/beam_runner_api_pb2.py100.00% <0.00%> (ø)
...sdks/python/apache_beam/transforms/environments.py85.32% <0.00%> (+0.13%)⬆️
...hon/apache_beam/runners/worker/bundle_processor.py94.40% <0.00%> (+0.41%)⬆️
...eam/runners/portability/fn_api_runner/execution.py95.14% <0.00%> (+1.45%)⬆️
...ks/python/apache_beam/runners/worker/data_plane.py91.31% <0.00%> (+1.79%)⬆️
...ild/srcs/sdks/python/apache_beam/runners/common.py90.63% <0.00%> (+1.91%)⬆️
...eam/runners/portability/fn_api_runner/fn_runner.py92.10% <0.00%> (+2.23%)⬆️
...on/apache_beam/runners/dataflow/dataflow_runner.py84.94% <0.00%> (+2.65%)⬆️
...rcs/sdks/python/apache_beam/transforms/timeutil.py98.88% <0.00%> (+3.43%)⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c6904d...c159f45. Read the comment docs.

@TheNeuralBit

TheNeuralBit commented Feb 26, 2021

Copy link
Copy Markdown
MemberAuthor

Ok, turns out the conflicts weren't trivial, because additional suppression were added in the meantime. I found the following worked:

# Remove any lines with just nullness TODO(BEAM-10402)
sed -Ei '/^\s*"nullness"\s*\/\/\s*TODO\(https:\/\/issues.apache.org\/jira\/browse\/BEAM-10402\)\s*$/d' **/test/**/*.java
# Run spotless, then remove empty SuppressWarnings annotations
./gradlew spotlessApply
sed -Ei '/@SuppressWarnings\(\{\}\)/d' **/test/**/*.java

Then I ran grep -IirnEl 'BEAM-10402' **/test/**/*.java to identify the last ~10 that needed to be cleaned up manually.

This is committed in ebe1105

@kennknowles

Copy link
Copy Markdown
Member

LGTM. If you hit a problem removing the suppressions, might as well get the first commit in sooner, to speed up builds for anyone who experiences cache misses.

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

Yeah I'll back out the suppression removal for now. It looks like it really did break Java PreCommit in a surprising way.

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

blurgh. The surprising schemas breakage is caused by the first commit, probably the strange nullable annotation behavior again..

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

if I'm being honest I don't quite understand why this works, but #14107 fixes this PR.

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

Perhaps running checkerframework brings in the spotbugs annotations at compile time?

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

Retest this please

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

Run GoPortable PreCommit

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

Run Java PreCommit

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

Run Java_Examples_Dataflow PreCommit

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

Run GoPortable PreCommit

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

Oh. GoPortable PreCommit no longer exists.

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

Run Java PreCommit

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

Run Java_Examples_Dataflow PreCommit

@TheNeuralBit
TheNeuralBit merged commit d378d57 into apache:masterMar 3, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@TheNeuralBit@kennknowles