[PAN-2586] Reference tests fail on Windows #1401
Conversation
ethereum/core/build.gradle
Outdated
| String testFile = testJsonFile.getName() | ||
| if(pathstrip == "BlockchainTests/"){ | ||
| String generalStateTests = "GeneralStateTests/" | ||
| if(testJsonFile.getParentFile().getParentFile().getName().toString() == generalStateTests){ |
There was a problem hiding this comment.
This condition will never fire. The name of the parent file will never end in a path separator ("/"). You need to test for "GeneralStateTests" and then concat "GeneralStateTests/"
There was a problem hiding this comment.
nice catch. thanks. this was a result of my overzealous beautification, I've reimplemented your proposed changes and run it again on windows to ensure that it's got the right test coverage
ethereum/core/build.gradle
Outdated
| paths << pathstrip + generalStateTests + "/" + parentDirectory + testFile | ||
| } | ||
| String transitionTests = "TransitionTests" | ||
| if(testJsonFile.getParentFile().getParentFile().getName().toString() == transitionTests){ |
There was a problem hiding this comment.
It looks like we are losing all tests that are not TransitionTests or GeneralStateTests, such as bcInvalidHeaderTest. Instead of doing a one-off for each test directory can these two if blocks be made to work for any parent name? maybe -
paths << parentstrip + testJsonFile?.parentFile?.parentFile?.name + "/" + parentDirectory + testFile
There was a problem hiding this comment.
I can see why you would say that, but that's not how it works -- Transition and General State are themselves subdirectories of Blockchain Test - both of which contain still other directories, the only ones like that, so if the test came from one of those two folders we need to add that to the path. You'll see what I mean if you check out the project structure there
There was a problem hiding this comment.
I have checked out the project structure, which is why I know this PR results in us not running all the tests.
Check out tech.pegasys.pantheon.ethereum.vm.blockchain.BlockchainReferenceTest_0 line 21 used to read
private static final String[] TEST_CONFIG_FILE_DIR_PATH = new String[] {"BlockchainTests/bcForgedTest/bcForkBlockTest.json", "BlockchainTests/bcForgedTest/bcForkUncle.json", "BlockchainTests/bcForgedTest/bcInvalidRLPTest.json", "BlockchainTests/bcMultiChainTest/CallContractFromNotBestBlock.json", "BlockchainTests/bcMultiChainTest/UncleFromSideChain.json"};
It now reads
private static final String[] TEST_CONFIG_FILE_DIR_PATH = new String[] {""};
We are missing many subdirectories: bcBlockGasLimitTest, bcExploitTest, and 12 others. i.e. too many to reasonable enumerate with if blocks like this. Plus, if a new directories were to be added we would miss them if we only take enumerated directories. We should continue to take all the subdirectories of BlockchainTest as discovered and not as enumerated.
There was a problem hiding this comment.
If you look in the directory BlockchainTests/, there are only two subdirectories that contain directories themselves as opposed to -- just a bunch of tests.
Those directories are pantheon/ethereum/referencetests/src/test/resources/BlockchainTests/GeneralStateTests and pantheon/ethereum/referencetests/src/test/resources/BlockchainTests/TransitionTests.
You mentioned bcBlockGasLimitTest , see here:
it follows the pattern paths << pathstrip + "/" + parentDirectory + "/" + testFile
However, if you look here:
we see the pattern paths << pathstrip + "/" + testJsonFile.getParentFile().getParentFile().getName() + "/" + parentDirectory + "/" + testFile, for tests inside of subdirectories of pantheon/ethereum/referencetests/src/test/resources/BlockchainTests/GeneralStateTests.
The point about not hardcoding those two directories is valid, I've updated the pr to be more dynamic.
|
confirmed that latest update passes on windows |
PR description
Resolves path issues with running on Windows.
At the moment it seems we aren't evaluating
VMTests,TrieTests, orTransitionTests-- why is that?Fixed Issue(s)