paketo-buildpacks / executable-jar

A Cloud Native Buildpack that contributes a Process Type for executable JARs.
Apache License 2.0
15 stars 7 forks source link

Refactor BFS tests to use gomega #236

Closed c0d1ngm0nk3y closed 11 months ago

c0d1ngm0nk3y commented 1 year ago

Fixes https://github.com/paketo-buildpacks/executable-jar/issues/234

Summary

Checklist

c0d1ngm0nk3y commented 1 year ago

@matejvasek As you are the original author of these tests, some feedback would be great if I understood them right. Note that it is not finished yet, e.g. I have to better understand them to name things.

matejvasek commented 1 year ago

@c0d1ngm0nk3y I do not understand this part:

errOurs := fsutil.Walk(root, createWalkFn(&ourFiles))
Expect(errOurs).NotTo(HaveOccurred())

errTheirs := filepath.Walk(root, createWalkFn(&theirFiles))
Expect(errTheirs).NotTo(HaveOccurred())

How is it equivalent to:

if (ourFiles == nil) != (theirsFiles == nil) {
    t.Errorf("errors does not match, actual error: %#v, expected error: %#v", errOurs, errTheirs)
}

Maybe slightly better condition would be !errors.Is(errOurs, errTheirs) && !errors.Is(errTheirs, errOurs) or maybe errors.Unwrap(errOurs) != errors.Unwrap(errTheirs).

The point is to verify that our and stdlib walk func return same error, not that error is nil. Speaking of which: would you mind adding a test similar to "non-existent root folder" but whit walkFn that propagates error?

c0d1ngm0nk3y commented 1 year ago

How is it equivalent to:

It is not at all. That is right. The point is that I didn't understand that part at all. It is exactly one test case, right? So either there are errors in both cases or not. It was used in 2 test cases and both do not return an error, so this was kind of pointless, imho.

Or did I miss something?

Speaking of which: would you mind adding a test similar to "non-existent root folder" but whit walkFn that propagates error?

When simplifying the code like this, I had also the feeling that this is missing. I will try to add.

matejvasek commented 1 year ago

It was used in 2 test cases and both do not return an error, so this was kind of pointless, imho.

Originally that code should have been common for multiple parametrized tests, some of them were supposed to return error. In particular "non-existent root folder" test should have used error propagating walk function but mistakenly I used error masking walk function.

c0d1ngm0nk3y commented 11 months ago

@anthonydahanne I think I have addressed your comments. Can we get this merged? Just let me know if I oversaw something.

dmikusa commented 11 months ago

Looks good to me. ASCII diagram for the win!

@c0d1ngm0nk3y can you update this one more time? Thanks

anthonydahanne commented 11 months ago

I rebased on my local fork; everything went fine, so I went ahead and did it here