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

tests: enable core24 plugin tests #5209

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

sergiusens
Copy link
Collaborator

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox run -m lint?
  • Have you successfully run tox run -e test-py310? (supported versions: py39, py310, py311, py312)

Issue raised: #5207

Signed-off-by: Sergio Schvezov <[email protected]>
- Remove the dotnet test for core24 from build and run
- Move the dotnet plugin test to the plugin suite

Signed-off-by: Sergio Schvezov <[email protected]>
@sergiusens sergiusens requested review from tigarmo and bepri January 17, 2025 15:45
Copy link
Contributor

@bepri bepri left a comment

Choose a reason for hiding this comment

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

That's a lot of movin' around! 20.04 spread test failures seem to be happening on other branches, so likely unrelated.

@sergiusens
Copy link
Collaborator Author

@bepri I still need to move those other python tests!

@bepri bepri self-requested a review January 17, 2025 21:00
spread.yaml Outdated
tests/spread/core24-suites/plugins/:
summary: core24 plugin tests
systems:
- ubuntu-24.04*
Copy link
Contributor

Choose a reason for hiding this comment

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

do these tests build destructively?
One argument "pro" using something lower than 24.04 is that it lets us catch failures in classic snaps

SNAP/colcon_msg_package: colcon-msg-package

systems:
- ubuntu-22.04
Copy link
Contributor

Choose a reason for hiding this comment

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

these are using 22.04, is that on purpose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, but I will drop it from here and make the suite use 22.04

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fwiw, they could be handled by the global suite systems filter, which I switched to 22.04, but these tests will not work on non amd64 or arm64, so the filter is more specific

@bepri bepri requested review from tigarmo and removed request for bepri February 13, 2025 14:20
@bepri bepri linked an issue Feb 18, 2025 that may be closed by this pull request
@bepri bepri force-pushed the work/CRAFT-3138-core24-plugin-spread branch from 85292f9 to 0c323b0 Compare February 21, 2025 14:42
@bepri bepri added the squash label Feb 24, 2025
Copy link
Contributor

@tigarmo tigarmo left a comment

Choose a reason for hiding this comment

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

thanks! One thing that looks "unclean" on these tests in general is the existence of spread/plugins/v2 referring to core20, spread/plugins/craft-parts referring to core22 and spread/core24-suites/plugins for core24. Seems a bit too cryptic but I don't think we need to address that in this PR

#shellcheck source=tests/spread/tools/snapcraft-yaml.sh
. "$TOOLS_DIR/snapcraft-yaml.sh"
# set_base "../snaps/$SNAP/snap/snapcraft.yaml"
SNAPCRAFT_BUILD_ENVIRONMENT: host
Copy link
Contributor

Choose a reason for hiding this comment

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

How come this test now runs on host?

Copy link
Contributor

Choose a reason for hiding this comment

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

It always must've. The content of the test itself requires destructive mode in order for the text files in all of the override-* steps to be created. I'm guessing it being destructive was set by the suite settings or something previously

@@ -1,5 +1,8 @@
summary: Test manifest file creation

systems:
- ubuntu-24.04*
Copy link
Contributor

Choose a reason for hiding this comment

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

This test runs snapcraft --use-lxd, so presumably the host doesn't need to be noble. Is there a specific reason for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

It also runs --destructive-mode a couple lines later, so it needed to be noble to build core24 snaps

summary: simple python application
description: build a python application using core24
base: core24
confinement: strict
Copy link
Contributor

Choose a reason for hiding this comment

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

for the future we should also have classic versions of this test, for poetry, python and uv

Copy link
Contributor

Choose a reason for hiding this comment

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

this set of tests feels a bit redundant with those in build-and-run-hello now; should we address it in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a super noisy PR as-is and has been up for a while, but I'm happy to talk about doing this in another PR

@@ -78,3 +78,51 @@ jobs:
google:ubuntu-22.04-64:tests/spread/core22/remote-build \
google:ubuntu-24.04-64:tests/spread/core24/remote-build \
google:fedora-39-64:tests/spread/core24/remote-build:no_platforms
google:ubuntu-22.04-64:tests/spread/core24-suites/remote-build
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the difference between tests/spread/core24 and tests/spread/core24-suites?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

core24 is a suite in itself, we could play a dance and move everything from core24 into core24/general and then move everything from core24-suites into core24.

We seen plenty of times people adding spread tests that look good but never get run because it was a suite introduced in a suite.

Copy link
Contributor

Choose a reason for hiding this comment

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

we could play a dance and move everything from core24 into core24/general and then move everything from core24-suites into core24.

This seems reasonable, at least it would be less confusing. Looking at this from another perspective: how should one decide where to put a new test?
but I'm fine either way I suppose.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does tests/spread/core24-suites/remote-build exist? I just kicked off a scheduled spread test to see what runs.

https://github.com/canonical/snapcraft/actions/runs/13531381365

Copy link
Contributor

Choose a reason for hiding this comment

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

It does not! Thanks for catching.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if it is base specific, and potentially for multiple bases, I believe repeating here is worth it, we have not had more than 3 bases active at any given time

Copy link
Collaborator

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

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

Everything looks good but I'd like to see the scheduled spread test complete before landing

@@ -78,3 +78,51 @@ jobs:
google:ubuntu-22.04-64:tests/spread/core22/remote-build \
google:ubuntu-24.04-64:tests/spread/core24/remote-build \
google:fedora-39-64:tests/spread/core24/remote-build:no_platforms
google:ubuntu-22.04-64:tests/spread/core24-suites/remote-build
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does tests/spread/core24-suites/remote-build exist? I just kicked off a scheduled spread test to see what runs.

https://github.com/canonical/snapcraft/actions/runs/13531381365

@lengau lengau self-requested a review February 26, 2025 01:32
The kernel plugin is only available as an experimental plugin, which isn't supported by snapcraft in core24, so this test isn't possible to run.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate or copy core20 and core22 spread tests to core24
5 participants