-
Notifications
You must be signed in to change notification settings - Fork 190
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
Initial version of integration tests #330
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.
Awesome, thanks for the PR! Looks great on first look, but let me take a closer look in the next days when I have more time.
# windows-latest is windows-2019 which carries a pretty old version of ruby (2.5) | ||
# we need at least ruby 2.7 for the tests | ||
# instead of dealing with installing a modern version of ruby on 2019, we'll just use windows-2022 here | ||
os: [ubuntu-latest, windows-2022, macos-latest] |
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 we could also use the official ruby/setup-ruby
action instead, which is a little more clear in its intent, and requires no explanation.
- uses: ruby/setup-ruby@v1
with:
ruby-version: 2.7
But I'm also fine with keeping windows-2022
.
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.
Actually I didn't realize setting up Ruby would be that easy. I still have the travis/appveyor mindset where installing different versions of software requires complex arcane platform-specific scripts, but of course on github there should be a one-liner for that.
I'm now in favor of keeping windows-latest, as it's always suspicious (at least to me) when a single concrete version of an OS is added in the CI :)
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.
Well... I was too quick to praise the setup-ruby action. The problem is that it adds mingw in the path and this confuses CMake. We could still work around this, but not over-complicating the CI scripts is a better idea IMO.
So I'm reverting my commit and keeping windows-2022
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.
Darn, I also would have expected it to just work, but then I agree that windows-2022 is the way to go!
9a8f994
to
8aa2732
Compare
This reverts commit 8aa2732.
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.
Great work on the documentation and testing library, this looks like a great start for the integration tests! Looking forward to extend on this in the future.
This is the initial version of the integration test framework and several tests:
cmake
command-line arguments. Incomplete. Many more invariants to go.This is obviously a very minimal (to say the least) collection of integration tests, but my proposal is that we merge this as it is and then discuss what new tests to make and perhaps have more users join in in writing them.
I suspect that as we write more and more tests, the framework will change dramatically but what we have in this PR seems, at least to me, as a decent starting point.
Some points of interest:
windows-latest
towindows-2022
(later than latest 😄). The thing is thatwindows-latest
comes with ruby 2.5. This is a pretty old version and it cannot execute the tests as they are. Currently the effort port to 2.5 is minimal, but it may cause more problems in the future. The tests as they are can be executed with Ruby 2.7.0 or later and I propose that we support whatever is available inubuntu-latest
. (Installing a modern version of Ruby onwindows-latest
is of course possible, but I opted for the simplest decision).