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.

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

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

Merged
merged 28 commits into from
Aug 18, 2024

Conversation

Ao-senXiong
Copy link
Member

No description provided.

@Ao-senXiong
Copy link
Member Author

#827 is failing for the dataflow. Let me try this one with compiler options.

@Ao-senXiong
Copy link
Member Author

it did not fail for the junit and nonjunit task... let me try again...

@Ao-senXiong
Copy link
Member Author

Still don't fail, let's try again...

@Ao-senXiong
Copy link
Member Author

@wmdietl This PR did not fail on junit and nonjunit test for a few times with --info --stacktrace --scan options for gradle test... Seems very werid while other PRs are failing..

@Ao-senXiong
Copy link
Member Author

try again with only worker, looks like the problem is concurrency in gradle task execution.

Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

d30ebee has an additional failure in jspecify-reference-checker, even after sever re-runs. That test passed in the corresponding PR #821.
The jspecify-reference-checker also seems to pass in all recent PRs. I can't reproduce the issue locally either. Let's see what happens with the next PR I merge into master...

.github/workflows/dependency-review.yml Outdated Show resolved Hide resolved
@@ -12,4 +12,4 @@ export ORG_GRADLE_PROJECT_useJdkCompiler=21
source "$SCRIPTDIR"/clone-related.sh


./gradlew test -x javadoc -x allJavadoc --console=plain --warning-mode=all
./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.

@wmdietl
Copy link
Member

wmdietl commented Aug 16, 2024

Also note that #857 passed tests twice without these failures.
So maybe just updating to Gradle 8.10 fixes these issues?

@Ao-senXiong
Copy link
Member Author

Also note that #857 passed tests twice without these failures. So maybe just updating to Gradle 8.10 fixes these issues?

Do you clear the cache? Maybe we can merge #857 first and see if other PRs work or not?

@Ao-senXiong
Copy link
Member Author

@wmdietl I think master branch did not fail for junit and nonjunit. However, #827 still fails. I think we still need this change and I have updated the scripts with comments and link to issue.

@wmdietl
Copy link
Member

wmdietl commented Aug 17, 2024

#857 updated to gradle 8.10. All tests passed in the PR.
However, jspecify-reference-checker on JDK 21 fails on master. Is that test not actually running on the PR? It is listed in the executed tasks.
Do you understand why that test fails only on master?

I've restarted #827 and it failed again with ERROR: internal error in type processor! method typeProcessOver() doesn't get called. So this PR still seems to be useful to work around those problems.

Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

Thanks! I'll merge this now and in #859 I'll try to figure out why the reference checker test fails.

checker/bin-devel/test-cftests-junit-jdk21.sh Outdated Show resolved Hide resolved
checker/bin-devel/test-cftests-junit.sh Outdated Show resolved Hide resolved
checker/bin-devel/test-cftests-nonjunit.sh Outdated Show resolved Hide resolved
@wmdietl wmdietl changed the title Try to fix random failure in dataflow test Increase dependency review severity requirement and work around dataflow test failures Aug 18, 2024
@wmdietl wmdietl enabled auto-merge (squash) August 18, 2024 00:09
@wmdietl wmdietl merged commit 4b710d4 into eisop:master Aug 18, 2024
74 checks passed
Sign up for free to 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