Merged
Conversation
github-actions
Bot
added
the
type/enhancement
Contributor
|
Thanks so much for helping out!
Wild. I never see that error, but the linter is correct. The imports should be: stdlib, external dependencies, internal dependencies. You didn't introduce the error as part of this PR so I don't think it's worth your time, in this PR. See: #1187 (comment)
Please just ignore them. We definitely need to cleanup, our cleanup logic, there. 😅
After adding and committing, but before git push. |
aemengo
reviewed
Jun 10, 2021
importhuman
commented
Jun 17, 2021
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
dfreilich
reviewed
Jul 2, 2021
Member
dfreilich
left a comment
dfreilich
left a comment
There was a problem hiding this comment.
In terms of code, @importhuman , this looks superb. Fits with our coding styles, and is very clean and pristine. Good work!
In terms of UX, I'm a bit concerned that people may not understand exactly what this option is (I would have difficulty explaining when to use this or not to use it). @ekcasey or @natalieparellano , can we have some further explanation of how/why people would want to use this, which we can add to the flag description and make it a bit clearer?
Member
Author
|
@dfreilich Thanks! Trying to figure out the edge case now. Also, not working on the analyzer scenario in this PR, as suggested by @natalieparellano on slack. I don't really understand how to use myself either, I've been going along what was given in the description and further discussions. 😅 |
github-actions
Bot
added
the
type/chore
importhuman
commented
Jul 6, 2021
Member
Author
importhuman
left a comment
importhuman
left a comment
There was a problem hiding this comment.
Having some trouble with writing certain tests 😅
| @@ -1764,6 +1764,45 @@ func testAcceptance( | |||
| }) | |||
| }) | |||
|
|
|||
Member
Author
There was a problem hiding this comment.
Couldn't figure out how to write the acceptance test. This is based off the cache-image test, but failing.
Contributor
There was a problem hiding this comment.
Let's not bother with an acceptance test for this.
They're slower, require more resources, and are more likely to fail. We should reserve them for more critical code paths with more moving parts. Although this PR touches a few moving parts, we have plenty other acceptance tests that the reduce need for this one.
Member
Author
|
|
||
| lastCallIndex := len(fakePhaseFactory.NewCalledWithProvider) - 1 | ||
| h.AssertNotEq(t, lastCallIndex, -1) | ||
| // when("image is invalid", func() { |
Member
Author
There was a problem hiding this comment.
Tried to add a unit test to check for invalid images, but couldn't figure out how to pass an invalid image name in the test as it gets validated. Tried passing tags like "%%%", tags with other invalid characters, tags with multiple slashes, etc.
Contributor
There was a problem hiding this comment.
Once again, I'd rather not bother with a test for this 😅
A test to check that we're bubbling up the error would be of low value, imo. Such a test would be more valuable if it were our logic deciding what a valid/invalid image tag is.
In this case, I'd be comfortable having faith in our third-party library having its own tests. And then saving ourselves the keystrokes for every error check.
| h.AssertNil(t, command.Execute()) | ||
| }) | ||
| }) | ||
|
|
Member
Author
There was a problem hiding this comment.
Couldn't figure out this test case, get the following error message:
=== RUN TestBuildCommand/Commands/#BuildCommand/previous-image_flag_is_provided/--publish_is_true/image_and_previous-image_are_not_in_same_registry/fails build.go:130: Unexpected call to *testmocks.MockPackClient.Build([context.Background { index.docker.io/some/image:latest my-builder map[] map[] true false false [] [] <nil> { []} always . {{ []} {[] [] [] [] } map[]} -1 example.io/some/previous-image}]) at /Users/ujjwal/coding/pack/internal/commands/build.go:130 because: there are no expected calls of the method "Build" for that receiver
Contributor
There was a problem hiding this comment.
Mock assertions are always painful to read. In this case, it's complaining that your mock received a call, but your test setup did not have the relevant mockClient.EXPECT() line.
Stepping back for a second. This unit test doesn't feel appropriate here. The logic for actually creating the error is way down in internal/build/lifecycle_execution.go. It feels like there's a decision to make about moving the test down or bringing the logic up...
I'd much prefer that you bring this logic (and tests) up. In fact, I'd prefer them to be at: internal/commands/build.go. That way we don't unnecessarily pull images, perform downloads, etc and then the action will fail anyway.
I'm glad that you tackled the edge case for us though 🙂
Member
Author
There was a problem hiding this comment.
Again, I added the test here because it was mentioned in the issue. 😅
I believe I can add the logic to internal/commands/build.go, but I don't get how to move the tests. Aren't the tests for this file in internal/commands/build_test.go, which is the file we're talking about here? Or would the logic of checking for invalid previous-image passed remove the need for a test? 😅
cc: @jromero
But for the sake of better understanding, I'd like to understand the error message.
Mock assertions are always painful to read. In this case, it's complaining that your mock received a call, but your test setup did not have the relevant mockClient.EXPECT() line.
When I have the mockClient line, I got the error "expected error but got nil". I tried it with various registry names, but got the same result. For example, with previous image name example.io/some/previous-image and image name index.docker.io/some/image:latest. Apologies if this information is insufficient.
Also, thank you so much for helping me with this PR, really appreciate it 😄
Member
There was a problem hiding this comment.
Just sent you a DM on slack if you want to do a bit of pair programing to get past this little hurdle.
Signed-off-by: Ujjwal Goyal <importujjwal@gmail.com>
Signed-off-by: Ujjwal Goyal <importujjwal@gmail.com>
Signed-off-by: Ujjwal Goyal <importujjwal@gmail.com>
Signed-off-by: Ujjwal Goyal <importujjwal@gmail.com>
Signed-off-by: Ujjwal Goyal <importujjwal@gmail.com>
importhuman
force-pushed
the
importhuman
branch
from
d54a4c4 to
0d10007
Compare
github-actions
Bot
removed
the
type/chore
Member
Author
|
Some tests are failing in the Edit: I ran the tests for my changes in a backup branch I had without pulling the latest changes from the main branch (i.e. only my additions), and got the same test failures. However, I don't get any failures for the main branch itself, i.e. without my code. Following are the failed test results (for backup branch, without latest upstream changes). EDIT 2: Figured out the problem, pushing commit soon. (Leaving the test results as they were) |
Resolves buildpacks#897 Signed-off-by: Ujjwal Goyal <importujjwal@gmail.com>
Member
Author
|
@jromero How can I fix the tests failing at the "no test files" stage? Other than this, the PR should be good to go. I've put back the image validation portion inside the condition as it was before, otherwise the tests were failing since not all of them were provided with images. |
jromero
approved these changes
Aug 11, 2021
4 tasks
1 task
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
4 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.
Signed-off-by: Ujjwal Goyal importujjwal@gmail.com
Summary
Add
previous-imageflag for #897 with anImagesfield. I have started with the changes required to theanalyzephase, but am having trouble writing the test for it (passing when it should fail).Additional assistance required:
make format,make tidyormake prepare-for-pr, getting the error"./create_builder.go" contains more than 3 groups of imports.make prepare-for-pr. Does "before creating a Pull Request" mean before adding and committing?Output
Before
After
Documentation
Related
Concerns #897 (Doesn't resolve yet)