Skip to content

Commit

Permalink
Merge pull request #184 from h1395010/add-unit-test
Browse files Browse the repository at this point in the history 
Adds new unit test entries
  • Loading branch information
@amebel
amebel committed Nov 3, 2014
2 parents 0fda448 + c884a07 commit ae87c47
Showing 1 changed file with 438 additions and 1 deletion.
Loading

1 comment on commit ae87c47

@linas
Copy link
Member

@linas linas commented on ae87c47 Nov 12, 2014

Choose a reason for hiding this comment

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

I will not have the time to review this in any detail. I just glanced at it. Whether or not to merge this is between you guys.

However, as a general rule, we should not add to the unit tests unless:

a) the test is correct! That means its been 'linguist-verified'. Oh, and we are the more-or-less the linguists here, which means we (you) need to look at each of these cases, and really think about them, and ask yourself "is this really correct?"

b) either the test is passing now, or will be passing due to changes in the next few days. Adding failing test cases is bad form, as it now becomes unclear as to where the problem is: is the unit test incorrect, or is it the algs file? or LG?

Adding unit tests that fail is bad, it defeats the purpose of having them, which is to make sure that new changes do not break old results... If the tests are failing, that means there are bugs ... and these bugs need to be fixed .. and if the bug is in the unit test itself, how will we know that?

So really: both a) and b) are really important.

Please sign in to comment.