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

1049 fix daily blockchain workflow #1152

Merged
merged 101 commits into from
Sep 24, 2024
Merged

Conversation

bradbown
Copy link
Contributor

No description provided.

…ow' into 1049-fix-daily-blockchain-workflow

# Conflicts:
#	reference-tests/build.gradle
@@ -52,6 +58,11 @@ tasks.register('referenceBlockchainTests', Test) {
description = 'Runs ETH reference blockchain tests.'
dependsOn generateBlockchainReferenceTests

boolean isCiServer = System.getenv().containsKey("CI")
minHeapSize = isCiServer ? "8g" :"4g"
maxHeapSize = isCiServer ? "120g" : "8g"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this will work, there is 128GB ram on the runner and if you reserve 120G for jvm, there isn't any left for corset.

@@ -110,7 +110,7 @@ tasks.withType(Test).configureEach {
showSkippedStandardStreams false

// set to false to hide failed standard out and error streams
showFailedStandardStreams true
showFailedStandardStreams false
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you want to set this to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was when I kept having the CI pipeline being cancelled without explanation so in an attempt to save memory I was removing the logs in the meantime.

boolean isCiServer = System.getenv().containsKey("CI")
minHeapSize = isCiServer ? "8g" :"4g"
maxHeapSize = isCiServer ? "20g" : "8g"
jvmArgs '-Xmx512m', '-Xms128m'
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, Xms and Xmx are used to set min and max heap size which are already set using minHeapSize and maxHeapSize. I am not sure which one will take effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the xmx and xms args.

private static ch.qos.logback.classic.Logger getLogbackLogger() {
try {
org.slf4j.Logger slf4jLogger = null;
for (int i = 0; i < LOGBACK_POLL_ATTEMPTS; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these attempts needed? Is there way to alter the instantiating order of this Test watcher and the root logger so that the root logger will be available before the test watcher is instantiated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added because there were occasions when substituteLogger was being retrieved. Since adding this I have excluded some jars from conflicting dependencies so I'll test whether this is still required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like it is still required.


public static Stream<Arguments> getTestParametersForConfig() {
public static Stream<Arguments> getTestParametersForConfig()
throws ExecutionException, InterruptedException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doew generateTestParametersForConfig throw these exceptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was due to calling get() on a completable future, I've reworked the usage and this is no longer required.

@bradbown bradbown enabled auto-merge (squash) September 24, 2024 13:27
@bradbown bradbown merged commit 0c18450 into arith-dev Sep 24, 2024
5 checks passed
@bradbown bradbown deleted the 1049-fix-daily-blockchain-workflow branch September 24, 2024 13:49
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