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: fix test failures on Windows #175

Merged
merged 3 commits into from
Oct 17, 2023

Conversation

StoneDot
Copy link
Contributor

Issue #, if available:
#159

Description of changes:
This PR fixes the issue of test failures on Windows by introducing separate snapshots for Windows.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ryota-sakamoto
Copy link
Contributor

We need to consider two thing for Windows.

  1. How to conduct test

We can use Windows image on GitHub Actions. So we can iterate test cycle as well as Linux.
https://github.com/actions/runner-images

  1. How to update snapshot

We will need to update snapshot when the command format is updated as well as Linux. Important thing is how to update snapshot. The idea is that we make a comment like /snapshot on the PR which run bot to update snapshot behalf of us.

@StoneDot StoneDot marked this pull request as draft October 5, 2023 10:09
@StoneDot
Copy link
Contributor Author

StoneDot commented Oct 5, 2023

Based on the feedback from ryota-ksakamoto, I will include mentioned feature into this PR.

@StoneDot StoneDot force-pushed the fix-windows-trycmd branch 5 times, most recently from 8ed0b37 to ae29b1c Compare October 7, 2023 14:55
@StoneDot

This comment was marked as resolved.

@StoneDot StoneDot force-pushed the fix-windows-trycmd branch from ae29b1c to 3a063d3 Compare October 7, 2023 15:47
@StoneDot
Copy link
Contributor Author

StoneDot commented Oct 7, 2023

The sample of an execution of the bot;
StoneDot#15 (comment)

@StoneDot StoneDot force-pushed the fix-windows-trycmd branch 6 times, most recently from b1c0f35 to aa7e8f1 Compare October 7, 2023 17:25
@StoneDot StoneDot marked this pull request as ready for review October 7, 2023 17:32
@StoneDot StoneDot force-pushed the fix-windows-trycmd branch 4 times, most recently from cac7c97 to 864555c Compare October 9, 2023 04:55
@StoneDot StoneDot force-pushed the fix-windows-trycmd branch 3 times, most recently from e6be368 to 0815603 Compare October 9, 2023 05:28
@@ -27,7 +27,7 @@ jobs:
os: macos-latest
target: x86_64-apple-darwin
- name: windows
os: windows-2019
os: windows-2022
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use windows-2022 rather than windows-latest? As I can see, .github/workflows/bot.yml use windows-latest also other platform use -latest as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for mentioning it. Because I believe that a stable binary generation is preferable, I specify windows-2022 instead of windows-latest. However, I did not notice that other platforms use the latest version. It seems that changing to latest is preferable for unification. Thus, I modified it as window-latest. I have created an issue to discuss the preferable version to support at #186.
On the other hand, I do not have a good reason for using windows-latest in bot.yml. Do you have any opinion regarding bot.yml?

Copy link
Contributor

Choose a reason for hiding this comment

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

From my opinion, we need to unify image on all of workflow. Because, the snapshot is actually not to relate building binary itself but if the image is different we are hard to identify the cause when any issue occur.
So would you mind if we use windows-2022 instead of windows-latest on other workflow for now and will discuss it on the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I think your opinion makes sense. Due to the scope of this PR, I changed os to windows-2022 for both CI and bots for the Windows platform. Let's discuss this within #186 about other platforms.

.github/workflows/bot.yml Outdated Show resolved Hide resolved
@StoneDot StoneDot force-pushed the fix-windows-trycmd branch 2 times, most recently from 6543f37 to ef179f9 Compare October 10, 2023 16:32
@StoneDot
Copy link
Contributor Author

@ryota-sakamoto Thank you for reviewing it. Could you check the modifications and comments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants