Skip to content

Conversation

prezha
Copy link
Contributor

@prezha prezha commented Aug 12, 2025

fixes #21301

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 12, 2025
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 12, 2025
Copy link
Member

@medyagh medyagh left a comment

Choose a reason for hiding this comment

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

@prezha seems like you checcked in a file, please rebase :) so it doesnt stay in history

@@ -225,6 +226,9 @@ func TestDeleteAllProfiles(t *testing.T) {
// then tries to execute the tryKillOne function on it;
// if after tryKillOne the process still exists, we consider it a failure
func TestTryKillOne(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to make our Killing Func to work on windows?

Copy link

@bobsira bobsira Aug 25, 2025

Choose a reason for hiding this comment

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

Yes.The test relied on platform-specific process/Wait() behavior and/or error text ("killed"). Windows and POSIX terminate processes differently: POSIX signals (SIGHUP/SIGKILL) and their wait/exit semantics differ from Windows TerminateProcess. The test assumed Windows Wait() would return a non-nil error that contains "killed", but in your environment Wait() returned nil. That mismatch made the assertion fail. Pushing a fix for this in a bit

@@ -56,9 +56,8 @@ jobs:
run: choco install make -y
# TODO: add gopogh reports for unit tests too
- name: unit test
timeout-minutes: 5
timeout-minutes: 15
Copy link
Member

Choose a reason for hiding this comment

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

was it really needed 15 mins?

@prezha prezha force-pushed the fix-windows-unit-tests branch from 2cdb3ce to a758221 Compare August 14, 2025 19:55
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: medyagh, prezha

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@medyagh medyagh merged commit 5fed7f9 into kubernetes:master Aug 28, 2025
22 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Unit Tests Windows
4 participants