DM-7829: adapt LC viewer to use IRSA api #250
Conversation
…to load properties in general for unit test
|
|
||
| # Tests level of output - change to INFO/WARNING to have more logs for example. | ||
| # This set the level to SEVERE to all the classes using java.util.logging logger when running tests | ||
| java.util.logging.ConsoleHandler.level=SEVERE |
There was a problem hiding this comment.
Are there libraries using java logging within our test environment? If so, is there a way to configure it to use Log4J instead? I am afraid using both would be confusing.
There was a problem hiding this comment.
unfortunately third party libraries use logging instead so you need to add the level property to cover this logger.
| irsa.gator.service.periodogram.url = "http://bacchus.ipac.caltech.edu:9027/cgi-bin/periodogram/nph-periodogram_api" | ||
|
|
||
| // Periodogram API request parameter list definition, separated by space | ||
| irsa.gator.service.periodogram.keys = "x y alg step_method pmin pmax step_size peaks" |
There was a problem hiding this comment.
app.config are meant for properties that changes depending on the environment it's deployed to.
This does not look like one that will. For class level properties, we have a 'resources' directory in the same source package.
There was a problem hiding this comment.
Well, imagine that the dev/test API has different keys than OPS, i still want to control that.
| <div> | ||
| <div> | ||
| <div style={{height:'100%'}}> | ||
| <div style={{height:'100%'}}> |
There was a problem hiding this comment.
if you don't mind, will you remove the outer div? it's pointless to have it since it does not serve anything.
| } | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
This variable is hard to read. It's in json format when it does not need to and seem to be deeper than necessary.
Why not?
const plotConfig = {
periodogram: {x: 'Period', y: 'Power'},
peaks: {x: 'Power', y: 'Peak'}
}
const {periodogram, peaks} = plotConfig;
There was a problem hiding this comment.
Is funny, for me is rather easier to read the other way but is probably less lines to read so yes i can clean it up.
| <br/> | ||
| <span> <b>Period Step Method: </b></span> | ||
| <br/> <br/> | ||
| <ListBoxInputField initialState={{ |
There was a problem hiding this comment.
The style and layout of this panel need more work. In general, it's a bad idea to use
for layout because it's dependent on font-size. Also, the duplicate labeling used for grouping only add to the cluttering.
There was a problem hiding this comment.
Yes, and we will need a cleanup story once we decide the layout and form.
| 'step_method': fields.stepMethod.value, | ||
| 'step_size': fields.stepSize.value, | ||
| 'peaks': fields.peaks.value, | ||
| //'result_table': 'http://web.ipac.caltech.edu/staff/ejoliet/demo/vo-nexsci-result-sample.xml', //For now return result table for non-existing API |
There was a problem hiding this comment.
I find it easier to read if the prop name is not quoted. Anything in quotes will be highlighted as string when they are not.
| //let catname0 = get(FieldGroupUtils.getGroupFields(gkey), 'cattable.value', catTable[0].value); | ||
| const fields = FieldGroupUtils.getGroupFields(gkey); | ||
| const srcFile = tbl.request.source; | ||
| console.log(fields); |
There was a problem hiding this comment.
console.log should be removed. there's more in other part of this PR.
|
|
||
| public static void load(String propFile) throws IOException { | ||
| // If there are no file properties, no need to add anything | ||
| Properties props = System.getProperties(); |
There was a problem hiding this comment.
Typically, you want to merge system's properties at the end so you can override it via command line.
There was a problem hiding this comment.
i thought that's what AppProperties.loadClassPropertiesFromFileToPdb was doing... isn't it?
| e.printStackTrace(); | ||
| } | ||
| return null; | ||
| return "http://web.ipac.caltech.edu/staff/ejoliet/demo/AllWISE-MEP-m82-2targets-10arsecs.tbl"; |
There was a problem hiding this comment.
I strongly suggest test data be moved to firefly_test_data git repos. There is no way to guaranteed the integrity of the test when the data is not controlled.
There was a problem hiding this comment.
You are right but here is more an integration test than a unit test, you need to test it from reading a URL and not a local file. See:edu.caltech.ipac.firefly.server.query.lc.IrsaLightCurveHandler#isPost
In order to test local file upload, you can check out the other test:
edu.caltech.ipac.firefly.util.PostDownloadTest
There was a problem hiding this comment.
I guess you misunderstood. "http://web.ipac.caltech.edu/staff/ejoliet/demo/AllWISE-MEP-m82-2targets-10arsecs.tbl" is used as an input file to testPhaseFoldedCurve @test.
Instead of getting the file from an external source, it should be in our firefly_test_data repository.
|
Mostly minor things, but they should be fixed. |
|
that's because i forgot to add the file `src/firefly/config/app-test.prop' ;-). There should a default one. Sorry about that. Will add it to the commit together with the rest of the changes. |
This PR is about hooking up the IRSA API and adapt the UI form to get the information and compute the periodogram. I have included also 2 more properties in
app.configthat control the exposed API parameters and the url.I've also changed couple of classes to be able to control the level of the log4j and logging when running unit test. Unit test that extends from a
ConfigTestclass will load properties to set log levels and app configuration properties in runtime.See more detail in the ticket for the API reference.
In order to test: bring the LC app and load a raw table. Then go to periodogram tab and play around. Every change should give different periodogram profile.