Skip to content
This repository was archived by the owner on Dec 5, 2024. It is now read-only.

Fix/GitHub tests #1087

Merged
merged 11 commits into from
Jun 4, 2018
Merged

Fix/GitHub tests #1087

merged 11 commits into from
Jun 4, 2018

Conversation

zilm13
Copy link
Collaborator

@zilm13 zilm13 commented May 29, 2018

Cache made with properties file and storing downloaded files. According to log it's used on travis instead of download. We could try this solution. What do you think?

@zilm13 zilm13 requested a review from mkalinin May 29, 2018 21:05
@coveralls
Copy link

coveralls commented May 29, 2018

Coverage Status

Coverage decreased (-0.1%) to 55.9% when pulling a92be08 on fix/github-tests into b1eb9f5 on develop.

@@ -74,10 +81,9 @@

public static String loadJSONFromCommit(String filename, String shacommit) throws IOException {
String json = "";
if (!SystemProperties.getDefault().githubTestsLoadLocal())
json = getFromUrl("https://raw.githubusercontent.com/ethereum/tests/" + shacommit + "/" + filename);
json = getFromUrl("https://raw.githubusercontent.com/ethereum/tests/" + shacommit + "/" + filename);
Copy link
Contributor

Choose a reason for hiding this comment

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

This change makes it impossible to read tests from local folder

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to have a 3rd option like GitHubTests.useDiskCache

@zilm13
Copy link
Collaborator Author

zilm13 commented May 30, 2018

@mkalinin how about such approach?
It will be difficult to parse test-ethereumj.conf in gradle config because we are using typesafe Config format, not native Java properties. I guess it's doable, but should we?

@mkalinin
Copy link
Contributor

The incentive behind storing it in the config is to avoid frequent updates of .travis.yml and keep user away from passing commit hash when it calls prepareGithubTests.

@zilm13
Copy link
Collaborator Author

zilm13 commented May 31, 2018

@mkalinin review me, plz

Copy link
Contributor

@mkalinin mkalinin left a comment

Choose a reason for hiding this comment

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

A couple of cosmetic notes. Otherwise looks good 👍

throw ex
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

new line pls

# to determinate commit of Ethereum tests repo https://github.com/ethereum/tests
# which should be checked out in local repo directory
# EthereumJ is tested with files from this commit
GitHubTests.commit=7f638829311dfc1d341c1db85d8a891f57fa4da7
Copy link
Contributor

Choose a reason for hiding this comment

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

new line pls

@mkalinin mkalinin merged commit 6162302 into develop Jun 4, 2018
@zilm13 zilm13 deleted the fix/github-tests branch June 4, 2018 15:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants