Bug fix for handling Windows paths

[phiferd]

Bug fix for handling Windows paths.

I tried to limit the impact of this change on any other platforms. Many of the unit tests fail on Windows (even without this change) probably due to a similar path handling issue.

Please verify on other platforms that there is no negative impact.

Fix for #112

[codecov-io]

Codecov Report

Merging #116 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #116   +/-   ##
=======================================
  Coverage   97.56%   97.56%           
=======================================
  Files           6        6           
  Lines         370      370           
  Branches       84       84           
=======================================
  Hits          361      361           
  Misses          9        9

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cedd3ee...bbb918d. Read the comment docs.

[phiferd]

  70 passing (3m)
  1 failing

  1) app "before all" hook:
     Error: Timeout of 60000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

I can run testrpc-sc separately and the tests pass, but I haven't sorted out what's going on with this before all hook. If I just check for windows and skip the startup of testrpc-sc, I get a lot of other errors that don't make much sense to me. e.g.

    1) config with testrpc options string: should generate coverage, cleanup & exit(0)
    2) config with test command options string: should run test
    3) config racing test command: should run test after testrpc has started
    4) contract tests events: tests should pass without errors
    5) trufflejs specifies coverage network: should generate coverage, cleanup and exit(0)

  ...

    1) app config with testrpc options string: should generate coverage, cleanup & exit(0):

      AssertionError: script should not error
      + expected - actual

      -false
      +true

      at Context.it (test\app.js:80:7)

[cgewecke]

@phiferd Ok cool! I'll go through the code this evening and try to identify other places where we've failed to write cross platform paths, especially in the test fixtures. I'm looking at this as a guide.

One thing it suggests is that by making sure we always using path module's normalize, join and resolve methods everything should 'just work' without having to test for the OS. Do you have any thoughts about that?

Would be cool if CI had a windows OS we could check against . . . oh well. 🙂

[phiferd]

That looks like a good guide. I don't think it will be too bad to refactor. Happy to test it out on Windows if you put it into a branch. I would love to be able to have coverage running locally as it makes writing unit tests a lot easier.

[phiferd]

There's still an issue with this update which I didn't realize was incorrect (since I had never used this or Istanbul before). The output uses absolute paths rather than relative paths, and that seems to confuse Istanbul, so the generated reports aren't quite right. The css/js imports don't work, and the ".html" files all end up next to the actual ".sol" files.

I will update this PR with a fix, but a better solution is probably to rework the path handling throughout.

[phiferd]

Finally a working report on Windows! :)

[cgewecke]

@phiferd Fantastic. Thanks for doing this.

I'm going to run some of our E2E tests against it and make sure everything works ok. Are there any outstanding issues re: launching the tool on Windows? Do we also need a small section in the FAQ addressing that?

[phiferd]

Sure. Thanks for doing all the rest of the work. :)

As for Windows specific docs, I think the only thing different is that users will just have to run testrpc-sc separately because I haven't been able to get that part to work. Other than that, I think the process should work as described in the main documentation.

EDIT: One thing that trips me up frequently is that testrpc-sc needs to be run in the project directory. I am often developing on multiple projects at the same time, and it's natural for me to kick of testrpc in one place and just let it run. For testrpc-sc, this will cause the tests to fail with:

        There was an error generating coverage. Possible reasons include:
        1. Another application is using port 8555
        2. Truffle crashed because your tests errored

      Error: ENOENT: no such file or directory, open 'C:\Users\Someone\MyProject\allFiredEvents'
Exiting without generating coverage...

[cgewecke]

Sorry it's taken me forever to get to this, but I checked it out today and everything is solid. Works for CI and coveralls, works locally on a unix box. Just perfect. I'm going to limit its scope to app.js though, and leave the test path fixes for another PR someday. [Edit] Actually I'll leave those in for now and merge.

[cgewecke]

@phiferd Hi. Very sorry but yesterday I resolved a merge conflict here incorrectly. Because this PR was opened on your master branch, I've broken your fork at master. . .

If you look at the diff from bbb918d you'll see I forgot to integrate a critical piece of the PR. I'm fixing it here but thought I'd let you know so that you can undo the damage on your end.

Thanks again for the contribution. :)

[phiferd]

Thanks for the heads up. No problem.