search

elki-project / elki

ELKI Data Mining Toolkit
https://elki-project.github.io/
GNU Affero General Public License v3.0
781 stars 321 forks source link

Added Tests for #87 #90

Closed lapplislazuli closed 2 years ago

lapplislazuli commented 3 years ago

Good Morning,

I saw Issue #87 and I thought it would be easy to run some tests. Do you think the tests are good for it? When I run them on the buggy version I get Index-Out-Of-Range Exceptions, they pass after your changes.

kno10 commented 3 years ago

In this particular case, I believe the maintenance cost of the test may outweight the benefits of testing for this corner case. But that is a philosophical discussion, software testing enthusiasts will certainly disagree. The comment should be more specific - github is the third VCS that we use (CVS, then SVN, then GIT), and hence an issue number itself is not a unique identifier anymore. A comment "Regression test: ensure that corner case k=1 is handled correctly" is more informative on why this test was added.

If you look at the diff of your pull request, you'll notice that you used a different code formatter, that should be avoided to keep diffs minimal. ELKI includes an eclipse code formatter definition that should be used.

kno10 commented 2 years ago

closing as stale.