Closed nickavv closed 7 years ago
Merging #37 into master will decrease coverage by
0.66%
. The diff coverage is50%
.
@@ Coverage Diff @@
## master #37 +/- ##
============================================
- Coverage 85.47% 84.81% -0.67%
- Complexity 103 115 +12
============================================
Files 18 22 +4
Lines 365 395 +30
Branches 33 33
============================================
+ Hits 312 335 +23
- Misses 38 45 +7
Partials 15 15
Impacted Files | Coverage Δ | Complexity Δ | |
---|---|---|---|
...onaide/main/config/CoronaIdeMainConfiguration.java | 0% <0%> (ø) |
0 <0> (?) |
|
...de-main/src/main/java/com/coronaide/main/Main.java | 0% <0%> (ø) |
0 <0> (ø) |
:arrow_down: |
...ain/java/com/coronaide/ui/CoronaUIApplication.java | 0% <0%> (ø) |
0 <0> (?) |
|
...coronaide/ui/config/UIControllerConfiguration.java | 100% <100%> (ø) |
2 <2> (?) |
|
...va/com/coronaide/ui/controller/MainController.java | 100% <100%> (ø) |
2 <2> (?) |
|
.../coronaide/core/service/impl/DatastoreService.java | 81.98% <0%> (+2.7%) |
18% <0%> (+2%) |
:arrow_up: |
... and 1 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 7c1c705...c20acb6. Read the comment docs.
Definitely will add tests before merging, really opened this review to get your thoughts on the structure of everything and if there's anything you'd like to try changing or improving before we settle on a pattern for now, rather than for hoping to merge it as-is.
I did a little bit more here, and I did find a reasonable way - in my opinion - to get the UI stuff separated back out into its own project. As far as testing the JavaFX stuff goes, it looks like there are some libraries for it but they don't have TestNG support. I'm sure there's a way to do it and it's worth looking into but for now I don't think it's necessary? Let me know your thoughts on that obviously.
The project thing looks good - tests aren't completely necessary, but it would be nice if we could figure it out and get over that initial setup hump.
Another library is easier for UI, go for it - tests are already annoying, and if using a different framework for UI makes them easier, or is more familiar to people working with FX, I'm fine with a different test dependency
I created some tests for the UI using the JUnit and TestFX frameworks. unfortunately there are 2 problems:
It's because I can't get the Spring context in the tests, and I was wondering if you would check out the branch and see if you could finagle it with your Spring-might.
Having problems in Eclipse, attempting update of Java to see if that fixes it. As fro Gradle - that's because the default test setup has "useTestNg", which turns off the default junit stuff. You can override it back by putting "test { useJUnit() }" (with the same test logging stuff) in the ui project's build.gradle
build failing because the UI test is not headless and travis can't handle that. Investigating how to make it run headless rn
It's my turn to make a middle-of-the-night pull request! Got a very basic JavaFX UI running with minimal fuss, and after some pain managed to integrate it with Spring so that we can use dependency injection in the UI Controllers.