Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Increase dependency review severity requirement and work around dataflow test failures #852

Merged
merged 28 commits into from
Aug 18, 2024
Merged

Increase dependency review severity requirement and work around dataflow test failures #852

Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
16aa4f5
Use gradlew debug to see why dataflow fails
Ao-senXiong Aug 10, 2024
f58ec40
Move delete lines to doLast block and see if it works
Ao-senXiong Aug 10, 2024
dbf6172
Stacktrace for nonjunit test as well
Ao-senXiong Aug 10, 2024
babd5ee
Add --info --stacktrace --scan for both tests
Ao-senXiong Aug 10, 2024
243de01
It stops failing now...
Ao-senXiong Aug 10, 2024
53ba049
Remove the compiler options in nonjunit test and try
Ao-senXiong Aug 10, 2024
7b2200c
Add debug options to junit-jdk21 and nonjunit
Ao-senXiong Aug 10, 2024
140c4ea
Don't do scan for junit job
Ao-senXiong Aug 11, 2024
cf3ce31
Add back scan for junit job
Ao-senXiong Aug 11, 2024
8c8fb25
Merge branch 'master' into test-ci-dataflow
Ao-senXiong Aug 12, 2024
df893eb
Empty commit for CI
Ao-senXiong Aug 12, 2024
61e9e65
Empty commit for CI
Ao-senXiong Aug 12, 2024
a0c59ce
Empty commit for CI
Ao-senXiong Aug 12, 2024
706e599
Empty commit for CI
Ao-senXiong Aug 13, 2024
0d0aba7
Remove info for junit test
Ao-senXiong Aug 13, 2024
470e93e
Remove scan for junit test
Ao-senXiong Aug 13, 2024
c50f1a9
Remove stacktrace and add back scan for junit test
Ao-senXiong Aug 13, 2024
bfb0e24
Remove scan and add debug for junit test
Ao-senXiong Aug 13, 2024
9e2ac2e
Try with only one worker
Ao-senXiong Aug 13, 2024
867253c
Try with only one worker for all scripts
Ao-senXiong Aug 13, 2024
e1de461
Empty commit for CI
Ao-senXiong Aug 13, 2024
714d271
Only fail when the severity is critical
Ao-senXiong Aug 14, 2024
e3dd7be
Empty commit for CI
Ao-senXiong Aug 14, 2024
035e85d
Empty commit for CI
Ao-senXiong Aug 15, 2024
f76acf0
Update fail on severity to high
Ao-senXiong Aug 16, 2024
c717030
Merge branch 'master' into test-ci-dataflow
Ao-senXiong Aug 17, 2024
592eeb8
Update script with comment and issue link
Ao-senXiong Aug 17, 2024
9c4c12a
Apply suggestions from code review
wmdietl Aug 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/dependency-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,6 @@ jobs:
- name: 'Dependency review'
uses: actions/dependency-review-action@v4
with:
fail-on-severity: high
retry-on-snapshot-warnings: true
retry-on-snapshot-warnings-timeout: 600
5 changes: 3 additions & 2 deletions checker/bin-devel/test-cftests-junit-jdk21.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
export ORG_GRADLE_PROJECT_useJdkCompiler=21
source "$SCRIPTDIR"/clone-related.sh


./gradlew test -x javadoc -x allJavadoc --console=plain --warning-mode=all
# Adding --max-workers=1 to avoid random failures in Github Actions. alternative solution is to use --no-build-cache.
wmdietl marked this conversation as resolved.
Show resolved Hide resolved
# https://github.com/eisop/checker-framework/issues/849
./gradlew test -x javadoc -x allJavadoc --console=plain --warning-mode=all --max-workers=1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #858 I'm trying to disable the configuration cache instead. That is what we did manually to fix the flakiness, so hopefully that also works here.
There is some connectedness here, too: https://docs.gradle.org/current/userguide/performance.html#additional_configuration_cache_benefits says that the configuration cache Executes all tasks in parallel. So maybe disabling the configuration cache does more than just setting the max-workers to 1.

This doc says that by default the configuration cache is off, but probably our CI setup enables it somehow.
That doc also says that --parallel is off by default, but it's not clear to me what the relation between max-workers and parallel is... Are the workers still used in parallel even if parallel is off? Strange...

But maybe the combination here is better. Anyways, there is more documentation here https://docs.gradle.org/current/userguide/configuration_cache.html and https://docs.gradle.org/current/userguide/performance.html#enable_configuration_cache

I also note that in typetools PRs 6422 and 6564 the maxParallelForks option is set for tests instead. There is some Azure-specific logic. It's again unclear what the relation between parallel forks and max workers is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From #858 it seems to be the build cache, not the configuration cache, that is causing the problems. So is it better to only have 1 worker or have the build cache? Can you compare timing between the two fixes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From #858 it seems to be the build cache, not the configuration cache, that is causing the problems. So is it better to only have 1 worker or have the build cache? Can you compare timing between the two fixes?

sure, maybe let's wait the ci finishes and do rerun a few times. We can see the how long does the junit, non-junit and junit java 21 task takes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both this PR and #858 take about 11h 42m... Let's just keep the solution here.
Can you update the .sh files and add a comment before why we're adding the flag, maybe that disabling the build cache would be an alternative.
If this closes issue #849, let's add a changelog entry. Alternatively, we can keep #849 open and see whether we fix the actual build issue within dataflow tests at some point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the .sh files and add a comment before why we're adding the flag, maybe that disabling the build cache would be an alternative.

Sure.

If this closes issue #849, let's add a changelog entry. Alternatively, we can keep #849 open and see whether we fix the actual build issue within dataflow tests at some point.

I prefer to keep #849. I think the reason might be some task should be execute before dataflow:busyexpression.

5 changes: 3 additions & 2 deletions checker/bin-devel/test-cftests-junit.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@ SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
# shellcheck disable=SC1090# In newer shellcheck than 0.6.0, pass: "-P SCRIPTDIR" (literally)
source "$SCRIPTDIR"/clone-related.sh


./gradlew test -x javadoc -x allJavadoc --console=plain --warning-mode=all
# Adding --max-workers=1 to avoid random failures in Github Actions. alternative solution is to use --no-build-cache.
wmdietl marked this conversation as resolved.
Show resolved Hide resolved
# https://github.com/eisop/checker-framework/issues/849
./gradlew test -x javadoc -x allJavadoc --console=plain --warning-mode=all --max-workers=1
5 changes: 3 additions & 2 deletions checker/bin-devel/test-cftests-nonjunit.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
# shellcheck disable=SC1090# In newer shellcheck than 0.6.0, pass: "-P SCRIPTDIR" (literally)
source "$SCRIPTDIR"/clone-related.sh


./gradlew nonJunitTests -x javadoc -x allJavadoc --console=plain --warning-mode=all
# Adding --max-workers=1 to avoid random failures in Github Actions. alternative solution is to use --no-build-cache.
wmdietl marked this conversation as resolved.
Show resolved Hide resolved
# https://github.com/eisop/checker-framework/issues/849
./gradlew nonJunitTests -x javadoc -x allJavadoc --console=plain --warning-mode=all --max-workers=1
./gradlew publishToMavenLocal -x javadoc -x allJavadoc --console=plain --warning-mode=all
# Moved example-tests out of all tests because it fails in
# the release script because the newest maven artifacts are not published yet.
Expand Down