-
Notifications
You must be signed in to change notification settings - Fork 231
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
ci: deprecate AppVoyer integration #128
Conversation
Pull Request Test Coverage Report for Build 688216325
💛 - Coveralls |
.github/workflows/test.yml
Outdated
strategy: | ||
matrix: | ||
os: [macos-latest, windows-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.
For feature parity, I've added the matrix build here, but imo it's an overkill. We can reliably only use unit
on ubuntu-20.04
(or ubuntu-latest
for that matter) which does also upload code coverage.
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.
This is really cool that this is supported so easily. It does feel like it is overkill at this point, but it is a "nice to have". I am open to either option really.
This is interesting (and makes sense in a way), but the unit tests on windows fail due to carriage return instead of newline. See: https://github.com/git-chglog/git-chglog/runs/2177927696?check_suite_focus=true#step:4:50 |
So, do we update the test that is failing? We could consider the OS when performing the expectation with the generated output. Or we could check that Or do we deprecate the windows test? |
PS - ❤️ the branch name |
That seems to be the issue, but this feels a bit weird to mutate the output based on the underlying OS. That said, I haven't had enough time since I opened this PR to delve into this. But my first feeling is that we should not have these. Line 388 in 4d8b2b6
Although the difference betwen LF and CRLF might still be relevant to some folks. |
So, it seems this issue is directly related to how Go manages line endings on different platforms. In other words That said, now, I'm in favor of always normalizing to LF. Because the intended destination of the output is a markdown file to be checked in a repository and be shared with broader audience. With that there won't be any difference between running Also with this change there won't be any need to run the tests on separate platforms, as they will always behave the same way. |
Nice find @khos2ow. After your explanation, it makes sense that we would be seeing this. I support normalizing to LF in our case. Your point about the desired destination of the tool output is spot on. I vote for deprecating the Windows tests at the least. I recognize that Mac OS is close enough to Linux in a general sense, so it may be overkill to include a separate Mac OS test run. That said, if it is minimal impact, it would be nice to keep the Mac OS test run. Mac is a common development platform. |
On the second though, I want to take back my earlier suggestion. I now think we'd best not mutate line endings at all and leave it to discretion of user to decide where they want to run this and how the line ending should look like. That's mainly because of the followings:
That said, and if we all agree to this, there won't be any need to run the tests on different platforms per se. They would behave correctly and accordingly on each of those as long as the tests are passing. |
To me, that's the critical piece that makes me feel like this is something we don't need to worry about in these tests. If we had concerns about file permissions management or something that can differ drastically for each OS, I would have more concern. |
With the migration to GitHub Action, there's no need to continue to use AppVoyer to only test on Windows, as it's covered by corresponding new jobs. Signed-off-by: Khosrow Moossavi <khos2ow@gmail.com>
a55df81
to
94d3a9b
Compare
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.
In light of the discussion above, I've reverted back matrix test and removed normalizing s/\r\n/\n/
in tests (as it doesn't happen anywhere else in the code, and that seems a hack to make tests pass on Windows)
That said, actual AppVoyer integration needs to be disabled on repository level and corresponding status checks need to be disabled from "Branch protection rules" (which I do not have access to neither.)
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.
Thank you for this. I have disabled the appvoyer checks on the mainline branch. See the image below. I will remove the webhook integration from the repo as well. Great work @khos2ow !
What does this do / why do we need it?
With the migration to GitHub Action, there's no need to continue to use
AppVoyer to only test on Windows, as it's covered by corresponding new
jobs.
Check lists
Which issue(s) does this PR fix?
Fixes #126