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

fix(application): attrs & tests for build planner #4588

Merged
merged 8 commits into from
Feb 20, 2024

Conversation

lengau
Copy link
Contributor

@lengau lengau commented Feb 16, 2024

  • 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)

Copy link

codecov bot commented Feb 16, 2024

Codecov Report

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

Comparison is base (6ff3e0b) 88.83% compared to head (8d2ee23) 88.88%.
Report is 6 commits behind head on feature/craft-application.

Files Patch % Lines
snapcraft/application.py 88.23% 2 Missing ⚠️
Additional details and impacted files
@@                      Coverage Diff                      @@
##           feature/craft-application    #4588      +/-   ##
=============================================================
+ Coverage                      88.83%   88.88%   +0.04%     
=============================================================
  Files                            327      329       +2     
  Lines                          22033    22053      +20     
=============================================================
+ Hits                           19573    19601      +28     
+ Misses                          2460     2452       -8     

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

@lengau lengau requested review from mr-cal and syu-w February 20, 2024 15:01
snapcraft/application.py Outdated Show resolved Hide resolved
tests/unit/services/test_package.py Outdated Show resolved Hide resolved
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.

Tests are very well written.

tests/unit/test_application.py Show resolved Hide resolved
@tigarmo
Copy link
Contributor

tigarmo commented Feb 20, 2024

@lengau I think this is still broken (from the spread tests):

2024-02-20T15:08:47.5728588Z 2024-02-20 15:08:47 Error executing google:ubuntu-22.04-64:tests/spread/core24/architectures:env_var_match (feb201506-275200) : 
2024-02-20T15:08:47.5730645Z -----
2024-02-20T15:08:47.5731243Z + cd ./snaps/env-var-match
2024-02-20T15:08:47.5747095Z + [[ -e environmental-variables.txt ]]
2024-02-20T15:08:47.5748802Z ++ cat environmental-variables.txt
2024-02-20T15:08:47.5750465Z + eval 'SNAPCRAFT_BUILD_FOR="arm64"' snapcraft pack --destructive-mode
2024-02-20T15:08:47.5752717Z ++ SNAPCRAFT_BUILD_FOR=arm64
2024-02-20T15:08:47.5753514Z ++ snapcraft pack --destructive-mode
2024-02-20T15:08:47.5755936Z snapcraft internal error: ValidationError(model='SnapcraftBuildPlanner', errors=[***'loc': ('architectures',), 'msg': "every item must either be a string or an object for [***'build-on': 'amd64', 'build-for': 'amd64'***, ***'build-on': ['amd64', 'arm64'], 'build-for': 'arm64'***]", 'type': 'value_error'***])

@lengau lengau requested review from syu-w, mr-cal and tigarmo February 20, 2024 20:27
@syu-w
Copy link
Contributor

syu-w commented Feb 20, 2024

Not sure what happened, now spread: command not found

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! I mostly looked at the spread tests and I think the failures are all expected/unrelated

@sergiusens sergiusens merged commit 902b30f into feature/craft-application Feb 20, 2024
7 of 10 checks passed
@sergiusens sergiusens deleted the ca/build-planner branch February 20, 2024 22:42
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.

5 participants