Add CLI driver tests#20
Add CLI driver tests#20camerondurham wants to merge 39 commits intoamazon-ion:masterfrom camerondurham:cli-tests
Conversation
* Added Dockerfile to build minimal, multi-stage image * Updated README with build instructions Recording of local build/test: https://asciinema.org/a/Ql7Xc5fgxxvkFrjjcuMbR0npY Currently builds image size 85.8MB from local build on macOS ``` docker images REPOSITORY TAG IMAGE ID CREATED SIZE ion-cli 0.1.1 e81a9d863b95 49 minutes ago 85.8MB ```
Update ion-rs version, ion-c submodule and Dockerfile
* Added Dockerfile to build minimal, multi-stage image * Updated README with build instructions Recording of local build/test: https://asciinema.org/a/Ql7Xc5fgxxvkFrjjcuMbR0npY Currently builds image size 85.8MB from local build on macOS ``` docker images REPOSITORY TAG IMAGE ID CREATED SIZE ion-cli 0.1.1 e81a9d863b95 49 minutes ago 85.8MB ```
* update to latest ion-c commit * add rust fmt and use stable toolchain for build
There was a problem hiding this comment.
Thanks for putting this together!
I'm not sure how to address the coverage issue off-hand, but I'd love to get the rest of these changes in. If it's not too much trouble, could you:
- Remove the coverage workflow for now.
- Add
windows-2019as anos(windows-latestpointed to 2019 until about a month ago, now it points to 2022.) - Update the
ion-csubmodule to point to the same commit asmain. It's hard to tell what the changes to 28 files are at a glance; I think we can updateion-cto the latest commit in a separate PR. - Add a comment to the
run_itmethod that explains what it's doing at a high level. (What exactly is being tested? It's ok if it's a trivial/placeholder test, but it'd be nice to communicate that.)
.github/workflows/build.yml
Outdated
| if: github.event_name == 'push' || github.event.pull_request.head.repo.full_name != 'amzn/ion-cli' | ||
| strategy: | ||
| matrix: | ||
| # TODO: add windows after fixing build issues |
There was a problem hiding this comment.
If the build issue was with windows-latest, we can get by using windows-2019 in the meantime.
There was a problem hiding this comment.
Tried both windows-2019 (GH action failure error(s) here) and windows-latest (error(s) here) and both seemed to have errors running the CMake step, with some possibly relevant warnings and errors here. For what it's worth, also tried blindly updating the ion-c submodule to latest commit but failed with similar errors here.
This could be issues mentioned in #5 but I do not have enough experience with CMake or Windows builds to be sure.
There was a problem hiding this comment.
Thanks for giving it a shot. It sounds like that'll take a future deep dive to sort out. We can leave it out for the time being.
|
Thanks for reviewing! I committed changes for 1, 3 and 4. Hopefully my comment in Also, apologies for the nearly 40 commits in this: I should have fetched and merged from this repo differently! |
| command_name | ||
| ); | ||
| unreachable!(message); | ||
| unreachable!("{}", message); |
There was a problem hiding this comment.
Noticed you fixed this already in #21. Apologies for including too many things in this CR, I'll avoid this in the future. Would it be better if I send a new CR rebased on the main branch with only the commits to add the driver test and workflow?
There was a problem hiding this comment.
Would it be better if I send a new CR rebased on the main branch with only the commits to add the driver test and workflow?
That would be really helpful if you don't mind!
|
Closing in favor of #22 Was unable to successfully squash commits but have simplified new CR to only the test files, Cargo and Github workflow config to run tests. |
2 participants
Issue #, if available:
Attempt to implement #10 (CLI driver tests) and #11.
Description of changes:
This CR adds basic tests for the CLI driver, using linked pull request (partiql/partiql-lang-rust#45) as example.
Other changes:
unreachableto build for 2021 editionUnfortunately, I was unable to get coverage to work despite tests running successfully. The CI seems unable to find coverage files and I wanted to make this PR to see if other contributors had seen this issue. The workflow and Cargo run commands almost exactly match that of amzn/ion-rust coverage run but when running on my fork the coverage files are not found. Fails with the following error:
This issue in actions/grcov seems related with the root cause being hyphens in crate names: actions-rs/grcov#51. I could be wrong, possibly missing something in the
ion-rust/**/build.rsfiles but I did not see anything in the ion-rust crate to resolve this yet the coverage still succeeds. Apologies for not resolving this first, I have been unable to find why the coverage files are not generated, is there something the maintainers see that I might be missing? Am I running the tests incorrectly or not following a standard required forgrcov?If I am unable to resolve why coverage is not passing, I will remove the workflow file since the CI Build workflow will ensure tests pass. Just wanted to bring this up here since I was unable to resolve find a solution yet.
Thank you!
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.