Skip to content

test: update workflow#1851

Merged
crazy-max merged 9 commits intodocker:masterfrom
crazy-max:test-flow
May 30, 2023
Merged

test: update workflow#1851
crazy-max merged 9 commits intodocker:masterfrom
crazy-max:test-flow

Conversation

@crazy-max
Copy link
Copy Markdown
Member

See individual commits.

I keep the test bake target for now. I think it's still useful to run unit tests without make or shell script.

@crazy-max crazy-max force-pushed the test-flow branch 2 times, most recently from c80265a to a178b7c Compare May 29, 2023 23:22
crazy-max added 9 commits May 30, 2023 02:51
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Also switch test target to run the test script.

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max crazy-max requested a review from jedevc May 30, 2023 08:13
@crazy-max crazy-max marked this pull request as ready for review May 30, 2023 08:13
Comment thread hack/test

if [ "$TEST_KEEP_CACHE" != "1" ]; then
docker rm -v $cacheVolume
trap 'docker rm -v $cacheVolume' EXIT
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

FYI - this was pulled from buildkit's ./hack/test, so maybe we need an update there as well?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes indeed I was looking at making some changes in BuildKit related to this 👍

Comment thread hack/test
Comment on lines -28 to -38
while test $# -gt 0; do
case "$1" in
integration)
TEST_INTEGRATION=1
;;
*)
echo "unknown arg $1"
;;
esac
shift
done
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think I'd like to keep this if possible, we still default to integration if nothing else is specified.

I think we're going to need to create another suite of integration tests at some point to test the complexities of driver creation, e.g. there are a lot of options for setting up the kubernetes driver. These tests don't test the commands package directly, so it would make more sense to split it out, so we can run them in parallel etc.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think I'd like to keep this if possible, we still default to integration if nothing else is specified.

On BuildKit we needed this special case because we got to test the frontend specifically and also the gateway and I don't think we need to have this kind of logic in buildx.

I think we're going to need to create another suite of integration tests at some point to test the complexities of driver creation, e.g. there are a lot of options for setting up the kubernetes driver. These tests don't test the commands package directly, so it would make more sense to split it out, so we can run them in parallel etc.

Right we just invoke buildx binary for the inspect command integration test atm so we don't cover the commands package but don't think we need other kind of invocation of the integrations tests container like we do in BuildKit.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

On BuildKit we needed this special case because we got to test the frontend specifically and also the gateway and I don't think we need to have this kind of logic in buildx.

I think our special case will come from wanting to test the commands specifically, and then also test the driver creation specifically. We'll probably want different types of environment for both (e.g. the driver creation tests will probably need to configure buildkit servers with different combinations of tls options, or spin up unique k8s clusters per test).

We might not do that though, so I think it's ok to simplify this for now, and we can always add it back in later.

@crazy-max crazy-max merged commit f178800 into docker:master May 30, 2023
@crazy-max crazy-max deleted the test-flow branch May 30, 2023 08:58
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