search

dmusican / Elegit

A GUI client for people who want to learn Git.
MIT License
30 stars 7 forks source link

Created Test Count Class #587

Closed kevinamick closed 6 years ago

kevinamick commented 6 years ago

Test class that counts number of tests in elegitfx package and fails if greater than one.

dmusican commented 6 years ago

@kevinamick , thank you so much for doing this! It's fantastic that you are jumping in to help out.

Your test is working so well that it caught a problem with one of our other unit tests files: TreeLayoutFXTest.java has two tests inside it. For whatever reason, TestFX wasn't breaking on that file, but we nonetheless should have just a single test in there since TestFX might have trouble with it at some point. That's a great find.

Here's the one catch: I don't want to merge this into develop until all unit tests pass; by design, we keep the develop branch in a state where all tests pass. I fully admit that the reason a different test is now failing is because your test is working. That said, since I don't want to merge this and cause tests to start failing, would it be reasonable for me to ask you to modify TreeLayoutFXTest.java as well as part of the same pull request so that it has just one test? I think you could just get away with combining those two tests and making them one; alternatively, you could make it two different classes with a test in each. (That's more of a pain, though, because of the shares setUp code, which shouldn't be duplicated.)

kevinamick commented 6 years ago

@dmusican, no problem! Happy to help. I'll dig into modifying the TreeLayoutFXTest class tomorrow so that it only uses one test.

kevinamick commented 6 years ago

@dmusican Ok, i've combined the tests in TreeLayoutFXTest. The build still seems to be failing on a SSH key test. I believe this is unrelated to my code, right?

dmusican commented 6 years ago

@kevinamick Agreed, it's not related to your code. I'm working on it. Stay tuned. :)

dmusican commented 6 years ago

Thanks, @kevinamick ! The failing unit test was a known instability of TestFX. I added a rule to the test (all of them, actually) that the TestFX team recommended, which made the problem go away, though it might have gone away anyway... some of those tests sporadically fail due to threading-related issues.

Anyway, your fix is in, and I really appreciate your help. Feel free to jump in and help on future issues.