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

feat!: fail if we know the project is core24 #4557

Merged
merged 5 commits into from Feb 8, 2024

Conversation

tigarmo
Copy link
Contributor

@tigarmo tigarmo commented Feb 1, 2024

The purpose of this PR is two-fold:

  1. Add breaking code that checks whether the project is known to be core24, to fail early. Otherwise, if the project has a different base (or if there's no project) then we fallback to the current codepath as usual.
  2. Add a set of core24 spread tests, each with an expected failure.

A lot of the changes are copied over from existing tests, so some commits can mostly be skimmed. I tried to split them up in a way that makes functional sense (to me at least):

  1. First commit has the actual feature of checking the base and adding a new RuntimeError;
  2. Second commit adds core24 spread tests, but as a straight-up copy of the existing tests in tests/spread/core22/. These tests are unchanged and still have core22 as base, so they don't need a big review;
  3. Third commit has the mostly-automated change of base of the core24 tests. I think it's worth a look over because I had to do a few manual changes;
  4. The fourth commit adds all expected failures in these new tests because none of them passes cleanly. For example, all tests that call snapcraft pack are expected to fail with "pack" command is not implemented for core24. The tests now check for this message and exit.
  5. Finally, the fifth commit restricts the CI spread runs to only tests/spread/core24. I confirmed that the other tests are passing, and restricting this feature-branch to only core24 tests speeds up the development loop considerably.

@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2024

Codecov Report

Attention: 34 lines in your changes are missing coverage. Please review.

Comparison is base (9855085) 88.33% compared to head (0abe95e) 88.20%.

❗ Current head 0abe95e differs from pull request most recent head d8567cd. Consider uploading reports for the commit d8567cd to get more accurate results

Files Patch % Lines
snapcraft/application.py 0.00% 28 Missing ⚠️
snapcraft/commands/unimplemented.py 0.00% 6 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@                      Coverage Diff                      @@
##           feature/craft-application    #4557      +/-   ##
=============================================================
- Coverage                      88.33%   88.20%   -0.13%     
=============================================================
  Files                            327      327              
  Lines                          21961    21992      +31     
=============================================================
  Hits                           19399    19399              
- Misses                          2562     2593      +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tigarmo tigarmo force-pushed the CRAFT-2441-craft-app-core24-only branch 6 times, most recently from f793720 to 4fb3c6b Compare February 6, 2024 11:35
@tigarmo tigarmo changed the base branch from main to feature/craft-application February 6, 2024 11:35
@tigarmo tigarmo force-pushed the CRAFT-2441-craft-app-core24-only branch from 4fb3c6b to 1372d07 Compare February 6, 2024 12:13
@tigarmo tigarmo marked this pull request as ready for review February 6, 2024 16:16
Base automatically changed from feature/craft-application to main February 7, 2024 11:10
This commit updates the scaffolding to have a different behavior in
cases where we *know* that the existing project (from the yaml file) is
core24-based.

In these cases, we now raise a RuntimeError outlining the name of the
command instead of raising a ClassicFallback error to let the
non-craft-application workflow take over.

With this, Snapcraft runs on core24 projects will fail until the
commands are ported over, while projects for older bases (or commands
that don't use projects, like "init") will still fallback to the
existing behavior.
These are all the tests from tests/spread/core22, copied over to
tests/spread/core24. Completely unmodified, they therefore still have
the core22 base.
This is a semi-manual update of "base" to "core24", "build-base" to
"devel" and "grade" to "devel" (where applicable).
This commit does the equivalent of pytest's "xfail(strict=True)": all
core24 tests are currently fail because no Snapcraft command has been
implemented yet, so the test checks that the tool fails with the
expected message, and then exits successfully.

The idea is that once a command is implemented the corresponding spread
tests will start to fail - we can then scrutinize each test to validate
the new behavior.
@tigarmo tigarmo force-pushed the CRAFT-2441-craft-app-core24-only branch from e76e2fd to 0abe95e Compare February 7, 2024 11:31
@tigarmo tigarmo changed the base branch from main to feature/craft-application February 7, 2024 11:31
@tigarmo
Copy link
Contributor Author

tigarmo commented Feb 7, 2024

I had to force-push this to re-seat it on the new feature/craft-application branch. Sorry!

@tigarmo tigarmo requested review from lengau and syu-w February 7, 2024 20:49
@tigarmo tigarmo requested a review from mr-cal February 8, 2024 15:26
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.

Excellent work! Looks like we will have to adjust the required CI jobs for this branch

tests/spread/core24/architectures/task.yaml Show resolved Hide resolved
The intention here is to speed-up spread runs and make the development
loop faster. I've run the non-core24-tests in this feature branch and
can confirm that they pass (or have unrelated failures).
@tigarmo tigarmo force-pushed the CRAFT-2441-craft-app-core24-only branch from 0abe95e to d8567cd Compare February 8, 2024 18:58
@tigarmo
Copy link
Contributor Author

tigarmo commented Feb 8, 2024

Excellent work! Looks like we will have to adjust the required CI jobs for this branch

thanks! I updated the workflow to keep the name of the required step

@tigarmo tigarmo merged commit b22fe93 into feature/craft-application Feb 8, 2024
9 of 10 checks passed
@tigarmo tigarmo deleted the CRAFT-2441-craft-app-core24-only branch February 8, 2024 19:19
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.

None yet

4 participants