Skip to content

Conversation

@myndzi
Copy link

@myndzi myndzi commented Sep 22, 2015

This PR includes a merge from #17 since the tests wouldn't pass without it.

@konobi
Copy link
Owner

konobi commented Sep 22, 2015

mmm... I like it! Can you change the package.json file in the test folder to either be blank "{}" or remove it altogether. Since the data is being supplied within the test already, it's best not to have a redundant copy.

@myndzi
Copy link
Author

myndzi commented Sep 22, 2015

That confused me for a while. I don't know why the file exists if its contents are ignored. I'd rather NOT mock the data and access the real file than have an ignored file.. what do you think?

@konobi
Copy link
Owner

konobi commented Sep 23, 2015

The tests are already written with mocks in place... the file is there for the existence check.

@myndzi
Copy link
Author

myndzi commented Sep 23, 2015

Seems like it makes sense to just read the file and skip the entire mocking step tbh, but I'll update it.

@konobi
Copy link
Owner

konobi commented Sep 23, 2015

The problem then is that you have to have a test file and directory for each test case. You would also then have to read each file to see what the test is actually looking for. With the tests mocked as they are currently, the tests are readable from within the test script itself instead across a variety of files.

@myndzi
Copy link
Author

myndzi commented Sep 23, 2015

Well, if the goal is to not perform integration tests against the file system and fs module, you're probably best off mocking the entire fs portion instead of half-mocking it and having directories laying about. But that's a change for a different PR.

@konobi
Copy link
Owner

konobi commented Sep 24, 2015

Mmmm... now I'm not sure how to deal with the scoring on this.

@konobi
Copy link
Owner

konobi commented Sep 24, 2015

I'm thinking that it should probably wrapped up into one test, which checks for the existance of "directories.test" or "./test/".

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