Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

170 Remove obsolete code #196

Merged
merged 3 commits into from Nov 25, 2015
Merged

170 Remove obsolete code #196

merged 3 commits into from Nov 25, 2015

Conversation

olaurino
Copy link
Member

Resolve #170

This PR removes obsolete and unused code both in the main source code base and in the test-components module, especially for components that had a large footprint. We can put them back at a later time if we want to.

Some of the removed classes were part of prototypes that did not turn into actual implementations and were mistakenly folded in when we were working with svn.

Some were experimental components that we shipped with Iris but that did not have enough traction and were not even documented. This applies in particular to the R component.

I tried to change the pom configuration so that jacoco excludes the test-components from the coverage analysis altogether. However, coveralls already seems to be ignoring such package, maybe because it matches a regular expression and gets ignored. In any case coveralls seems to be missing a lot of coverage anyway.

On Sonar I get a 0.6% increase in coverage (to 36.1%) which seems consistent with the amount of code removed and with the fact that the test-components were already ignored in the sonar configuration.

@eholum
Copy link
Member

eholum commented Nov 18, 2015

Merge it.

@jbudynk
Copy link
Member

jbudynk commented Nov 18, 2015

For the most part, looks good. I checked that the test-components site shows the updated information; it does.

But this testing show how quickly we should get the SAMP/sherpa-samp IT tests up and running, because there are errors in the SedStacker code that were introduced before this PR :)

I tested the SedStacker code by commenting the"@ignore" tags in each SedStacker*Test.java. There were some errors, most likely due to the updated JUnit version and the new GUI test infrastructure. There are some windows I expect to pop-up in the SedStacker tests; the error message shown below suggests we need to use the WindowInterceptor to handle the pop-ups. The other errors have to do with not providing tolerances in assertEquals(). Apparently this was OK in previous junit versions.

Failing tests:

JUnit assertEquals():

Running cfa.vo.sed.science.stacker.SedStackerNormalizerTest
java.lang.AssertionError: Use assertEquals(expected, actual, delta) to compare floating-point numbers
    at cfa.vo.sed.science.stacker.SedStackerNormalizerTest.testNormalizer(SedStackerNormalizerTest.java:270)

Running cfa.vo.sed.science.stacker.SedStackerRedshifterTest
java.lang.AssertionError: Use assertEquals(expected, actual, delta) to compare floating-point numbers
    at cfa.vo.sed.science.stacker.SedStackerRedshifterTest.testRedshifter(SedStackerRedshifterTest.java:279)

Running cfa.vo.sed.science.stacker.SedStackerStackerTest
java.lang.AssertionError: Use assertEquals(expected, actual, delta) to compare floating-point numbers
    at cfa.vo.sed.science.stacker.SedStackerStackerTest.testStacker(SedStackerStackerTest.java:268)

GUI errors:

testNormalizerOutsideRange(cfa.vo.sed.science.stacker.SedStackerNormalizerTest)  Time elapsed: 4.054 sec  <<< ERROR!
java.lang.Error: Unexpected window shown - this window should be handled with WindowInterceptor. 
        ....
    at cfa.vo.iris.gui.NarrowOptionPane.showMessageDialog(NarrowOptionPane.java:44)
    at cfa.vo.sed.science.stacker.SedStackerNormalizer.normalize(SedStackerNormalizer.java:229)
    at cfa.vo.sed.science.stacker.SedStackerNormalizerTest.testNormalizerOutsideRange(SedStackerNormalizerTest.java:356)

testRedshifterNoZ(cfa.vo.sed.science.stacker.SedStackerRedshifterTest)  Time elapsed: 4.023 sec  <<< ERROR!
java.lang.Error: Unexpected window shown - this window should be handled with WindowInterceptor. 
        ....
    at cfa.vo.iris.gui.NarrowOptionPane.showMessageDialog(NarrowOptionPane.java:44)
    at cfa.vo.sed.science.stacker.SedStackerRedshifter.shift(SedStackerRedshifter.java:184)
    at cfa.vo.sed.science.stacker.SedStackerRedshifter.shift(SedStackerRedshifter.java:69)
    at cfa.vo.sed.science.stacker.SedStackerRedshifterTest.testRedshifterNoZ(SedStackerRedshifterTest.java:363)

@jbudynk
Copy link
Member

jbudynk commented Nov 18, 2015

I think this PR is fine to merge so long as we make a task to fix the SedStacker errors.

@olaurino
Copy link
Member Author

We already have a task for that, i.e. #172. Also, this may be related to
#192 and might be addressed as part of the review for #195.

@eholum
Copy link
Member

eholum commented Nov 18, 2015

The JUnit assert stuff was fixed in bc412ec , still more to do for removing the @ignore flags though.

@olaurino olaurino changed the title Remove obsolete code 170 Remove obsolete code Nov 18, 2015
@olaurino
Copy link
Member Author

Rebasing. The original tip was at dd36181

olaurino added a commit that referenced this pull request Nov 25, 2015
@olaurino olaurino merged commit 38ca5c0 into master Nov 25, 2015
@olaurino olaurino deleted the remove-obsolete-code branch December 9, 2015 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants