-
Notifications
You must be signed in to change notification settings - Fork 173
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
lorri2isis Test Conversion #4286
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks solid! Just one quick question about a test conversion
commands: | ||
$(APPNAME) from=$(INPUT)/lor_0030710290_0x633_sci_1.fit \ | ||
to=$(OUTPUT)/lor_0030710290_0x633_sci_1.cub \ | ||
> /dev/null; | ||
catlab from=$(OUTPUT)/lor_0030710290_0x633_sci_1.cub \ | ||
to=$(OUTPUT)/lor_0030710290_0x633_sci_1.pvl > /dev/null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason that this didn't get converted? I would assume because the summing modes don't effect the import app and the main logic for getting the summing mode is in the translation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@acpaquette I was not sure how this test was different than the tests I already implemented. If you think it'll be useful to add it in, let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was testing a translation file specific look up. If you checkout this translation file the it looks for a particular key for the summing mode.
If the app was doing anything with the summing mode I would suggest reimplementing the test as a gtest. As you said this test isn't doing anything directly with the app so I think we can keep it as is. Good job on this PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Test Conversion for lorri2isis app
Description
Converted lorri2isis/main.cpp to lorri2isis/lorri2isis.cpp and lorri2isis.h. Added 7 gtests.
Related Issue
Test coversions.
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Licensing
This project is mostly composed of free and unencumbered software released into the public domain, and we are unlikely to accept contributions that are not also released into the public domain. Somewhere near the top of each file should have these words: