-
Notifications
You must be signed in to change notification settings - Fork 0
Add integration test with verified BOM and submodules #238
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
base: main
Are you sure you want to change the base?
Conversation
Coverage SummaryTotal Project Coverage
Coverage by File
Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
assert len(bom) == 14 | ||
assert bom == csv_snapshot | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty similar to the test below it. Should we replace that one or is it testing something your new test doesn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this test I added the design reuse repo as a submodule. When we support submodules via the project data API I plan on updating this particular integration test by removing the explicit design reuse repo reference.
That being said, I can close this PR if it's not worth merging and just include this test in the upcoming submodule support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth testing both submodule as well as the explicit link for external sheets.
Can you add a comment in the test stating such?
If I understand it right: "TODO: Remove this explicit url once py-allspice / project.json supports submodules"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth testing both submodule as well as the explicit link for external sheets.
There are a couple other tests that use the explicit repo reference that I won't change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have duplicate tests doing the same thing, then we should remove them.
I think that makes sense - this test isn't "red" because we're adding the reuse repo to the list, making it hard to tell what the point is right now. |
Changes
TODO