Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests refactoring #66

Merged
merged 11 commits into from
Apr 4, 2016
Merged

Tests refactoring #66

merged 11 commits into from
Apr 4, 2016

Conversation

oleggromov
Copy link
Contributor

A bunch of changes:

  • trying to remove as much repetition as possible (particularly in test initialization) by moving initialization into beforeEach blocks
  • grouping tests in logical groups
  • removing test environment (test file existence) tests — there's no need to test environment together with the code
  • replaces stream with a buffer in a test case name

Questions

Seems like reading-writing test files in every test case is rather useless and, even worse, makes tests code too complicated. Ideally, each test case should consist only of one check, but I don't know yet how to achieve such simplicity. Maybe you have any thoughts?

I also wonder why are the file parameters such as cwd and base needed. Once I remove them nothing happens, but I don't sure if this is a regression-safe change.

@lazd
Copy link
Owner

lazd commented Mar 3, 2016

So the cwd and base arguments are part of the Vinyl API. They do have sensible defaults, and since we're not using those values in our assertions, it's probably fine to remove them.

@lazd
Copy link
Owner

lazd commented Mar 3, 2016

We could probably abstract reading the test files into a function that just matches the file contents against a path, something like fileMatches(file, 'path'), but we'll have to read in the expected output at some point.

We're not actually writing output, just getting the Vinyl file object from the stream, so that's fine.

@oleggromov
Copy link
Contributor Author

Hi Larry, thanks for feedback!
I recently removed unnecessary options and tried to simplify each test case even more by moving check function outwards to the beforeEach block. Although there are a lot of other ways to brush tests' code and even make checks more precise, maybe we can leave this improvement as is by now?

In fact I hadn't had a plan to refactor tests, but started writing code that consistently solves #65 problem and found that I need to improve tests readability in order to complete my PR with written tests for the new code.

@lazd
Copy link
Owner

lazd commented Mar 4, 2016

Yeah, this is good. We could spend all day making tests look simpler, but this is a great improvement as-is.

Can you do me a favor and double check that there are no false positives as a result of this refactor? Simply invert every expect and make sure they all fail. Sometimes the asynchronous nature of a test gets overlooked in these kinds of refactors, and it ends up passing with flying colors when it ought to fail. If it passes that little audit, then we're good to merge.

@oleggromov
Copy link
Contributor Author

It's all arranged!
I'll perform additional check on each test to be certain of it correctness and write to you once all checks finish.

@oleggromov
Copy link
Contributor Author

Hi Larry. I've just checked if every test fails with inverted assertion and this is true. I've also become convinced of the test verifying finish triggering also fails once the event is renamed.

@oleggromov
Copy link
Contributor Author

@lazd What do you think about this PR? I need it to complete #67 too.

@lazd
Copy link
Owner

lazd commented Apr 4, 2016

Let's merge it!

@lazd lazd merged commit 65b5deb into lazd:master Apr 4, 2016
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