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

Conversation

fgimenez
Copy link
Contributor

@fgimenez fgimenez commented Jun 9, 2018

Related to #21

Adds general test structure inspired by ethereumjs-block and a small test similar to the tests in py-evm. More tests should be added for the first task in #21, mostly proposing now for getting feedback about the approach, directory structure and file naming, testing on filesystem, how mocks are defined, etc.

I've added tmp as an explicit dependency too, let me know if that's ok (it's not required, was already pulled by other deps).

@holgerd77
Copy link
Member

Cool, thanks for the PR

@fgimenez fgimenez force-pushed the data-dir-test branch 2 times, most recently from 1d7e9ff to 8d0602e Compare June 9, 2018 12:14
@coveralls
Copy link

coveralls commented Jun 9, 2018

Pull Request Test Coverage Report for Build 37

  • 39 of 41 (95.12%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-9.8%) to 90.196%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/chain/DBManager.js 21 23 91.3%
Totals Coverage Status
Change from base Build 25: -9.8%
Covered Lines: 39
Relevant Lines: 41

💛 - Coveralls

@fgimenez
Copy link
Contributor Author

fgimenez commented Jun 9, 2018

@holgerd77 np :) Regarding the error on 6.14.2 https://travis-ci.org/ethereumjs/ethereumjs-client/jobs/390066687 looks like in that case the Blockchain object created in ChainManager constructor is giving an error. I've just pushed some changes to inject it and extracting all the DB functionality to a separate class, let me know WDYT.

@holgerd77
Copy link
Member

Hi @fgimenez, I am now back from holidays, thanks again for the PR. I am getting the following error when running this (with an additional directory console.log output for info added to the test file:

> npm run lint && node tests/


> [email protected] lint /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-client
> standard

TAP version 13
# [DB]: Database functions
# should test data dir creation
/var/folders/7t/7nxvnvbn3hqgydbs00wr088r0000gn/T/tmp-50917zkmvozamWEF0/chaindb
/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-client/lib/chain/DBManager.js:34
          throw err
          ^

Error: EISDIR: illegal operation on a directory, mkdir '/'
    at Object.fs.mkdirSync (fs.js:891:18)
    at targetDir.split.reduce (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-client/lib/chain/DBManager.js:31:12)
    at Array.reduce (<anonymous>)
    at DBManager._mkDirByPathSync (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-client/lib/chain/DBManager.js:28:26)
    at DBManager._initializeDatadir (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-client/lib/chain/DBManager.js:43:12)
    at new DBManager (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-client/lib/chain/DBManager.js:11:10)
    at Test.<anonymous> (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-client/tests/db-manager.js:16:5)
    at Test.bound [as _cb] (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-client/node_modules/tape/lib/test.js:77:32)
    at Test.run (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-client/node_modules/tape/lib/test.js:96:10)
    at Test.bound [as run] (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-client/node_modules/tape/lib/test.js:77:32)

Do you know/see what is going on here?

@fgimenez
Copy link
Contributor Author

hey @holgerd77 looks like on OSX the error returned when trying to create an existing dir is EISDIR instead of EEXIST. I've pushed changes to check for the existence of the directory before trying to create, instead of trying to create and checking the error, please give it another try.

This will be probably less performant than the first approach but more cross-platform, IMO in this case (creating the initial datadir) the lack of performance is not so important, let me know WDYT.

@fgimenez
Copy link
Contributor Author

hi @holgerd77 did you have a chance to take a look at the latest changes? Let me know if this approach is ok or we should try something different.

I've been thinking about having a way to catch the issue you experienced, do you think it would be useful to enable OSX builds on travis config?

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Hi Federico,
this looks good and now also works on my OSX machine, will now merge.

Hmm, don't see why we shouldn't run Travis tests also on OSX, so we might just want t add this to a Travis build matrix. I think we shouldn't detail this out too much with the different systems we target, think there is some time for this to be a task later on, .

Holger

@holgerd77 holgerd77 merged commit beedc72 into ethereumjs:master Jun 25, 2018
@fgimenez fgimenez deleted the data-dir-test branch June 25, 2018 13:44
@fgimenez
Copy link
Contributor Author

Cool, makes total sense, thanks! :)

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