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

Builder tests #3555

Merged
merged 1 commit into from
Oct 18, 2024
Merged

Builder tests #3555

merged 1 commit into from
Oct 18, 2024

Conversation

apostasie
Copy link
Contributor

@apostasie apostasie commented Oct 16, 2024

This PR focused solely on rewriting builder tests with the new test framework.

Any other changes (in testutil) are removal of no-longer used helpers, and making sure we can compile on other platforms as we make test files move away from _platform.go patterns.

Besides 1-1 rewrite, a few test changes are worth noting:

  • (re)introducing a non-native platform build test
  • making some tests previously ran only on linux to also run on windows
  • also note that we no longer perform the builder prune op that was previously in place. The key upside is that tests can now be ran in parallel instead of sequentially, with no downside as far as I can tell (if there is any, we can always --no-cache where it matters)

This PR has been reduced as much as feasible to not contain any other changes.

I know it is still big (1k lines)... - lmk if you think there is a good way to break this up.

@apostasie apostasie changed the title [WIP] Part 7: builder tests Part 7: builder tests Oct 16, 2024
@apostasie apostasie marked this pull request as ready for review October 16, 2024 19:33
@apostasie apostasie marked this pull request as draft October 16, 2024 19:35
@apostasie apostasie force-pushed the part7 branch 19 times, most recently from 2379346 to e212720 Compare October 17, 2024 19:46
@apostasie
Copy link
Contributor Author

Failures are fixed by #3563

@apostasie apostasie changed the title Part 7: builder tests Builder tests Oct 17, 2024
// and adds cleanup steps to test cleanup. The builder name is returned as output.
//
// If not docker, this function returns an empty string as the builder name.
func SetupDockerContainerBuilder(t *testing.T) string {
Copy link
Contributor Author

@apostasie apostasie Oct 17, 2024

Choose a reason for hiding this comment

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

This was used only once and could be simplified in the test in-place.

@@ -43,7 +43,9 @@ const (
NginxAlpineImage = "registry.k8s.io/e2e-test-images/nginx:1.14-2"
NginxAlpineIndexHTMLSnippet = "<title>Welcome to nginx!</title>"

GolangImage = "fixme-test-using-this-image-is-disabled-on-windows"
GolangImage = "fixme-test-using-this-image-is-disabled-on-windows"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we remove more of platform-specific test files and replace with platform-dependent t.Skip, we need a few placeholders so that we compile.

The point of this (as exposed ealier), is to make platform-specific skip/unskip easier than it is today.

Signed-off-by: apostasie <[email protected]>
@apostasie apostasie marked this pull request as ready for review October 17, 2024 22:53
@apostasie apostasie marked this pull request as draft October 17, 2024 23:15
@apostasie apostasie marked this pull request as ready for review October 17, 2024 23:17
@ktock ktock merged commit c4eab4d into containerd:main Oct 18, 2024
22 checks passed
@apostasie
Copy link
Contributor Author

Thanks a lot @ktock

@AkihiroSuda AkihiroSuda added this to the v2.0.0 milestone Oct 18, 2024
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.

3 participants