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

Test for parsing CLI arguments in lfc #1668

Merged
merged 12 commits into from
Apr 4, 2023
Merged

Test for parsing CLI arguments in lfc #1668

merged 12 commits into from
Apr 4, 2023

Conversation

patilatharva
Copy link
Collaborator

This PR adds a test that tests if the CLI arguments passed into Lfc match the values in the target properties Properties object which is passed to the generator. The test only checks that the desired values are passed to the generator and doesn't test the functionality of those values, because I believe that would belong in the LF unit tests and be out of scope for the CLI tests.

Rationale: #1641 - The bug fixed in #1631 showed that our current CLI tests are merely smoke tests and not sufficient to detect if arguments are passed properly.

Implementation notes: I preserved the structure of the existing CliToolTestFixture.java as much as possible, but chose to test by creating an Lfc object rather than using static methods. It seemed like the simplest way to gain visibility into the instance methods (which are necessary for Picocli), but let me know if there's a better way around this in Java.

@patilatharva patilatharva marked this pull request as ready for review March 20, 2023 05:59
@patilatharva patilatharva linked an issue Mar 20, 2023 that may be closed by this pull request
Copy link
Collaborator

@oowekyala oowekyala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this on! I will push a couple of changes in the direction of my comments

@patilatharva
Copy link
Collaborator Author

@oowekyala thanks for the changes, it looks much better now!

@patilatharva
Copy link
Collaborator Author

@oowekyala just added a small fix to make the test ./gradlew test --tests org.lflang.tests.cli.LfcCliTest.testBuildParams pass - I instantiated lfc with injector.getInstance so the correct io would be passed in.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good! I left some minor requests for changes. Perhaps beyond the scope of this PR but good to do in the future would be to extend the tests to check the resulting TargetConfig as well.

org.lflang.tests/src/org/lflang/tests/cli/LfcCliTest.java Outdated Show resolved Hide resolved
org.lflang.tests/src/org/lflang/tests/cli/LfcCliTest.java Outdated Show resolved Hide resolved
org.lflang.tests/src/org/lflang/tests/cli/LfcCliTest.java Outdated Show resolved Hide resolved
org.lflang/src/org/lflang/cli/Lfc.java Outdated Show resolved Hide resolved
@lhstrh lhstrh changed the title Testing arguments' values in Lfc. Test for parsing CLI arguments in Lfc Mar 21, 2023
@lhstrh lhstrh changed the title Test for parsing CLI arguments in Lfc Test for parsing CLI arguments in lfc Mar 21, 2023
@patilatharva patilatharva requested a review from lhstrh April 4, 2023 01:17
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thanks, @patilatharva!

@lhstrh lhstrh merged commit 21876f7 into master Apr 4, 2023
41 checks passed
@lhstrh lhstrh deleted the cli-tests branch April 4, 2023 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test the arguments of all our command line tools
3 participants