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

client/windows integration tests: plumbing work for running on windows #4432

Merged
merged 2 commits into from
Dec 7, 2023

Conversation

profnandaa
Copy link
Collaborator

@profnandaa profnandaa commented Nov 22, 2023

This PR does most of the plumbing work required for us to start running integration tests on Windows. We will need this to land before we can do the follow-up PRs to handle the specific tests.

/cc. @gabriel-samfira

@profnandaa profnandaa force-pushed the windows/integration-tests branch 2 times, most recently from 63a40d1 to 95b60bf Compare November 22, 2023 09:15
cmd/buildkitd/main_containerd_worker.go Outdated Show resolved Hide resolved
util/testutil/integration/util.go Show resolved Hide resolved
@profnandaa profnandaa force-pushed the windows/integration-tests branch 6 times, most recently from c56c6d4 to e0946b6 Compare November 22, 2023 14:05
@profnandaa profnandaa marked this pull request as ready for review November 22, 2023 14:10
@tonistiigi
Copy link
Member

@profnandaa Some linter errors in CI.

Co-authored-by: Gabriel Samfira <[email protected]>

This PR does most of the plumbing work requred for us to start
running integration tests on Windows. We will need this to land
before we can do the follow-up PRs to handle the specific tests.

Signed-off-by: Anthony Nandaa <[email protected]>
@profnandaa
Copy link
Collaborator Author

@profnandaa Some linter errors in CI.

@tonistiigi -- fixed. was a build directive mismatch on one file linux vs. !windows.
For the remaining CI failture (1), this seems to be throttling issue?

util/testutil/integration/util_unix.go Outdated Show resolved Hide resolved
"testing"
)

func checkRequirement(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.

Not for this PR but looks like this function is not named correctly.

util/testutil/workers/sysprocattr_windows.go Outdated Show resolved Hide resolved
util/testutil/workers/sysprocattr_unix.go Outdated Show resolved Hide resolved
util/testutil/workers/util_windows.go Show resolved Hide resolved
util/testutil/workers/containerd.go Outdated Show resolved Hide resolved
profnandaa added a commit to profnandaa/buildkit that referenced this pull request Nov 30, 2023
The idea is to skip all tests for now, since they were originally meant
for Linux. Then we will comb through each, enabling them for Windows
and making necessary changes.

This will make sure that all tests are fully considrered for Windows, and some
do not just pass as false positives.

Depends on: moby#4432

Co-authored-by: Gabriel Samfira <[email protected]>
Signed-off-by: Anthony Nandaa <[email protected]>
@profnandaa profnandaa force-pushed the windows/integration-tests branch 6 times, most recently from 5396720 to c930371 Compare December 2, 2023 18:21
@profnandaa
Copy link
Collaborator Author

profnandaa commented Dec 2, 2023

You can hold review till I fix the failing CI.

Fixed, ready for review.

@profnandaa profnandaa force-pushed the windows/integration-tests branch 3 times, most recently from 8600d9a to 630b062 Compare December 4, 2023 08:33
@profnandaa profnandaa changed the title windows integration tests: plumbing work to be able to run on windows client/windows integration tests: plumbing work to be able to run on windows Dec 4, 2023
@profnandaa profnandaa changed the title client/windows integration tests: plumbing work to be able to run on windows client/windows integration tests: plumbing work for running windows Dec 4, 2023
@profnandaa profnandaa changed the title client/windows integration tests: plumbing work for running windows client/windows integration tests: plumbing work for running on windows Dec 4, 2023
Copy link
Collaborator

@gabriel-samfira gabriel-samfira left a comment

Choose a reason for hiding this comment

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

Sorry to be this late to the party, I've been travelling.

Just a couple of really small changes, but overall looks good. This will allow us to hook up the integration tests in the CI in subsequent PRs and start enabling tests.

util/testutil/workers/containerd.go Outdated Show resolved Hide resolved
util/testutil/workers/containerd.go Outdated Show resolved Hide resolved
util/testutil/workers/util.go Outdated Show resolved Hide resolved
NOTE: all these follow-up commits will be squashed after the review
is done.

What changed in this commit:

- refactor the code to make the dial retry logic be the same across
  platforms.
- a number of code duplication fixes and refactors

Signed-off-by: Anthony Nandaa <[email protected]>
@profnandaa
Copy link
Collaborator Author

@gabriel-samfira -- thanks for the review, fixed all the issues raised, ptal.

@tonistiigi tonistiigi merged commit bf84288 into moby:master Dec 7, 2023
59 of 60 checks passed
@profnandaa profnandaa deleted the windows/integration-tests branch December 7, 2023 19:17
@profnandaa
Copy link
Collaborator Author

Thanks for the merge! Will proceed to the next one towards having the integration tests CI enabled on Windows.

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

Successfully merging this pull request may close these issues.

5 participants