Code Quality Issue and Testing

[benpdavison]

Hello,

I recently cloned the repo and was looking through the code base. I noticed there is a lot of code issues and warnings that could lead to potential bugs, additionally, I could not see anywhere that listed how the code was tested when new features were added etc. I am more than happy to refactor code to improve general quality (most of it can be done automatically) but I am cautious about doing so if there arn't any tests inplace to ensure that it still functions as expected afterward.

Kin regards,

[DeviaVir]

related to: #229 I will close that one in favor of this description

[haxwell]

All.. any preference on testing framework? I was gonna get started on this..

[DeviaVir]

@haxwell I think mocha is one of the standard ones? But it really depends on what you're familiar with and what is fastest/easiest for you in this case. The rest of us can/should adapt.

[haxwell]

Okay, I will go with Mocha. +1

Just a heads up, there's going to be some big changes in the structure of the code. For example, take the commands (backfill, trade, etc). Each of the commands are similar, in that 1) there is one big function which implements the command. That big function is going to have to broken down into smaller functions. 2) Also when the big functions do have smaller functions within them, there is a lot of sharing of state between those smaller functions. That state should be passed in to the function, for ease in mocking functionality in tests.

For those two reasons, among others, change-is-a-comin'.. Just wanted to give a heads up, so the PR is not so scary.

PS, I plan to start with just one command (probably backfill), and move gradually elsewhere.

Context: https://www.toptal.com/javascript/writing-testable-code-in-javascript

[haxwell]

Actually, I went with Jasmine, because it was easier to create mock objects. This effort begins in PR #974.

[JensvdHeydt]

@haxwell I was looking into some js testing frameworks. How do you feel about switching from jasmine to jest? I think it's a little bit more capable and it supports snapshot testing against data structures. That feature could be very handy in refactoring the codebase.

[haxwell]

@JensvdHeydt Jest is another good framework, certainly.. I think the snapshot feature (and perhaps Jest in general) is geared towards UI testing, whereas we would need it for testing how state changes between blocks of code. So, once we get to higher level testing (features, rather than functions) Jest might be useful. For this effort, I think Jasmine is a better fit.

[JensvdHeydt]

@haxwell Actually Jest snapshot testing works with much more than just html
from their docs: "Note: While snapshot testing is most commonly used with React components, any serializable value can be used as a snapshot" So you can actually use it with arrays and even objects.
Also have a look at this: https://medium.com/powtoon-engineering/a-complete-guide-to-testing-javascript-in-2017-a217b4cd5a2a

[haxwell]

Just an update.. I am hard at work on a refactor of the Backfill functionality, with the intent to make it unit tested. I should have something to show within a week (worst case) in the coming days.

[haxwell]

See PR #1214, Refactor Backfill for Unit Testing. Please review and test!