movio / bramble

A federated GraphQL API gateway
https://movio.github.io/bramble/
MIT License
497 stars 55 forks source link

handle failed schema composition in merge test helper #138

Closed exterm closed 2 years ago

exterm commented 2 years ago

Schema composition can fail. If it does, it previously caused a panic through referencing a nil pointer.

In this case, it's better to return early.

I discovered this when experimenting with modifications to the schema composition logic, and instead of failing tests, I got the panic from the test helper.

lucianjon commented 2 years ago

Hi @exterm, good catch and thanks for the PR. So the testing library here has two packages to deal with assertions: assert and require. I find it a little odd but if you use an assertion from the assert package and the assertional fails, the test will continue on. If you use require as your assertion package, it will immediately fail the test. Personally I tend to just use require everywhere as it tends to cause less confusion.

In this case given the must prefix, I'd expect if the inner call errored out, the calling function wouldn't continue. I think it would be best to just replace assert.NoError(t, err) with require.NoError(t, err).

What do you reckon?

codecov-commenter commented 2 years ago

Codecov Report

Merging #138 (403ecf2) into main (70a8e42) will increase coverage by 0.03%. The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #138      +/-   ##
==========================================
+ Coverage   69.58%   69.61%   +0.03%     
==========================================
  Files          24       24              
  Lines        2630     2630              
==========================================
+ Hits         1830     1831       +1     
+ Misses        669      668       -1     
  Partials      131      131              
Impacted Files Coverage Δ
auth.go 87.42% <0.00%> (+0.62%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 70a8e42...403ecf2. Read the comment docs.

exterm commented 2 years ago

I'd expect if the inner call errored out, the calling function wouldn't continue.

Yes, that's what I would have expected! I'm just starting out with go and didn't know the libraries involved. Your proposed solution is clearly better. Will make that change.

lucianjon commented 2 years ago

Looks good, thanks @exterm