backfill integ test coverage for git ref modules#6693
Merged
jedevc merged 3 commits intodagger:mainfrom Feb 20, 2024
Merged
Conversation
This was panicking because parentSrc is not set when the sdk module itself is a git module (since parentSrc is irrelevant in that case). Signed-off-by: Erik Sipsma <erik@dagger.io>
Signed-off-by: Erik Sipsma <erik@dagger.io>
Signed-off-by: Erik Sipsma <erik@dagger.io>
jedevc
approved these changes
Feb 20, 2024
Contributor
There was a problem hiding this comment.
It's a bit annoying to maintain since you need to update the separate repo and then update the commit to use in the test code, but it's much better than not having this coverage at this point.
Agreed, but this is a very reasonable stop-gap IMO 🎉
Let's get this in ASAP, very little chance of breaking things, and better error messages are wonderful 🎉 We can do any follow-ups later.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #6623
Most of the recent bugs from the last week ended up being simple things related to using git module refs, which was due to the fact that we didn't have integ test coverage of those.
We didn't have coverage previously because it's slightly less obvious than it seems due to the limitation around git modules only being allowed to be from
github.com/. That essentially rules out authoring the tests in the repo itself (there's various shenanigans that might be possible but it gets way too complicated to be worth it).So, until we have support for non-github module refs, we can just maintain a separate git repo that has "test fixture" modules. I created one in my org here for now (should be moved to be under the dagger org asap).
It's a bit annoying to maintain since you need to update the separate repo and then update the commit to use in the test code, but it's much better than not having this coverage at this point.
There's a few changes to actual code here too:
Due to the fix for the panic, I will provisionally add this to v0.9.11, but if @jedevc or others would prefer to save this for later it's an obscure enough case that it can wait.
TODO (can be now or follow-up)