Fix: Memory leak by not disposing kernel in unit tests of feature #79 plus code quality #81

[CoreRasurae]

Feature #79: Renames the unit test LocalArrayArgsIssue79Test to Issue79LocalArrayArgsTest, so that it is no longer skipped, plus removes the memory leak by ensuring the kernel is disposed at the end.

Feature #81: Improves code quality by logically splitting long unit test class into two unit test classes, one with the base validations and another with the advanced validations. Also KernelRunner new methods are refactored to reduce code complexity by extracting methods, and reducing code duplication.

[codecov-io]

Codecov Report

Merging #83 into master will increase coverage by 0.07%. The diff coverage is 0%.

@@             Coverage Diff              @@
##             master      #83      +/-   ##
============================================
+ Coverage     46.15%   46.23%   +0.07%     
  Complexity      852      852              
============================================
  Files            57       57              
  Lines          9593     9578      -15     
  Branches       1568     1563       -5     
============================================
  Hits           4428     4428              
+ Misses         4706     4691      -15     
  Partials        459      459

[freemo]

@CoreRasurae Am I correct in understanding that while this fixes a bug in the unit test that there was no bug in the core code? The changes to core code are mostly cosmetic?

[CoreRasurae]

Yes, the bug was in the unit tests. The changes I made were based on the code quality report from the previous build.

[CoreRasurae]

Strangely it seems that more tests are skipped now on CI environemt, don't know why. On my machine after rebasing to 1.6.0, I was getting the tests LocalArrayArgsIssue79Test also getting skipped, the 2 tests, then I renamed to Issue79LocalArrayArgsTests and they were no longer skipped. Now on my machine I have this: [INFO] Running com.aparapi.runtime.Issue79LocalArrayArgsTest [INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.14 s - in com.aparapi.runtime.Issue79LocalArrayArgsTest [INFO] Running com.aparapi.runtime.Issue81AtomicsSupportAdvTest [INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.626 s - in com.aparapi.runtime.Issue81AtomicsSupportAdvTest [INFO] Running com.aparapi.runtime.Issue81AtomicsSupportTest [INFO] Tests run: 42, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.584 s - in com.aparapi.runtime.Issue81AtomicsSupportTest

However on the CI environment they still keep being skipped, even after renaming, while now the Issue81 tests are also being skipped... after having split those tests into two classes, one with the base tests and the other with the more complete/advanced ones.

[CoreRasurae]

Let me make codacy completely happy it just complaining about a not so important thing, but I can make it happy in a second.

[freemo]

@CoreRasurae Yea im kinda clueless why certain tests are being skipped myself. I tried looking into it before the last release. Since this PR doesnt address a bug int he core code I'll still merge it when its ready (let me know when its ready to merge) but i wont rush out a 1.6.1 release.

On Mon, Apr 16, 2018 at 10:28 AM, CoreRasurae notifications@github.com wrote:

Strangely it seems that more tests are skipped now on CI environemt, don't know why. On my machine after rebasing to 1.6.0, I was getting the tests LocalArrayArgsIssue79Test also getting skipped, the 2 tests, then I renamed to Issue79LocalArrayArgsTests and they were no longer skipped. Now on my machine I have this: [INFO] Running com.aparapi.runtime.Issue79LocalArrayArgsTest [INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.14 s - in com.aparapi.runtime.Issue79LocalArrayArgsTest [INFO] Running com.aparapi.runtime.Issue81AtomicsSupportAdvTest [INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.626 s - in com.aparapi.runtime.Issue81AtomicsSupportAdvTest [INFO] Running com.aparapi.runtime.Issue81AtomicsSupportTest [INFO] Tests run: 42, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.584 s - in com.aparapi.runtime.Issue81AtomicsSupportTest

However on the CI environment they still keep being skipped, even after renaming, while now the Issue81 tests are also being skipped... after having split those tests into two classes, one with the base tests and the other with the more complete/advanced ones.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/Syncleus/aparapi/pull/83#issuecomment-381617497, or mute the thread https://github.com/notifications/unsubscribe-auth/AC5JAisM9E7c2KE6PGGWaRuyXYmw08G3ks5tpKqJgaJpZM4TWmit .

[CoreRasurae]

@freemo Sure, I will send a message when it is ready. I think there is no need for a 1.6.1 too, it is just minor things. BTW I sent you an email, this morning, to you work email address that I got from github, did you receive it?

[freemo]

@CoreRasurae I see that email now, responding...

On Mon, Apr 16, 2018 at 10:34 AM, CoreRasurae notifications@github.com wrote:

@freemo https://github.com/freemo Sure, I will send a message when it is ready. I think there is no need for a 1.6.1 too, it is just minor things. BTW I sent you an email, this morning, to you work email address that I got from github, did you receive it?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Syncleus/aparapi/pull/83#issuecomment-381620498, or mute the thread https://github.com/notifications/unsubscribe-auth/AC5JAmdBlB8Oh7zqVdGNlRPh2-JnKmQ5ks5tpKwTgaJpZM4TWmit .

[CoreRasurae]

@freemo I'm done with the changes. You can merge if you want to.

[freemo]

@CoreRasurae wonderful, thanks so much. I will review and merge. by the way you still dont have your commits linked up to your github account so you arent getting credit for them. Not sure if this is intentional but just wanted to let you know.

[CoreRasurae]

I see, thanks! Not critical, anyway.

[freemo]

@CoreRasurae merged, thanks again!