ci: add Windows testing to CI #132
Conversation
.github/workflows/test.yml
Outdated
| if: ${{ matrix.os }} == 'windows-latest' | ||
| run: | | ||
| git config --global core.autocrlf false | ||
| git config --global core.eol lf |
There was a problem hiding this comment.
What test was failing if this was not included? I am of the opinion that if this is necessary in CI to get it to pass, then the repo itself is broken, and we should be changing the .gitattributes file so that using Windows is not broken for contributors in some subtle way.
There was a problem hiding this comment.
All tests fail on Windows (if using autocrlf) as the line endings are converted to CRLF. I did think about adding it to the .gitattributes, however wasn't sure if that was the best place. Guess we could add text eol=lf to line 1 of the .gitattributes, instead of text=auto, but that seems kind of wrong...
I've done a test branch here to try that out... 👍🏻
https://github.com/owenvoke/tldr-lint/actions/runs/1668369649
There was a problem hiding this comment.
@MasterOdin, looks like that also works. Shall I add that commit to this branch? 👍🏻
There was a problem hiding this comment.
Yes, please. If text eol=lf is necessary for the random Windows users to be able to run the tests successfully regardless of their configuration, then so be it. Better than including in the README a block on "if you want to work on this repo on windows, run these git config settings first..."
| @@ -19,7 +19,7 @@ jobs: | |||
| if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.id == github.event.pull_request.base.repo.id }} | |||
| with: | |||
| access_token: ${{ github.token }} | |||
|
|
|||
There was a problem hiding this comment.
What's the change here? My phone doesn't show anything, just want to confirm not adding spaces or something.
There was a problem hiding this comment.
It's removing whitespace (the same amount as the indent of the line above it).
There was a problem hiding this comment.
As waldyrious mentions, it's just removing whitespace that was included before. 👍🏻
There was a problem hiding this comment.
@MasterOdin, is this ok to merge?
Closes #129