detect extra fields in fixtures#33
Conversation
There was a problem hiding this comment.
Can we add some tests for abstract types? I don't see any handling of that in the implementation so I would expect these tests to fail
There was a problem hiding this comment.
Added. I refactored to track expectedFields across different types and added tests
lopert
force-pushed
the
lopert.extra-fixtures
branch
from
24d2e9d to
d078685
Compare
| } | ||
| } | ||
| possibleTypesStack.push(possibleTypes); | ||
| typeConditionStack.push(namedType ?? null); |
There was a problem hiding this comment.
This is a bit of an undefined vs null thing.
schema.getType can return undefined, but in selection sets I explicitly push null values to the typeConditionStack. I chose null to represent that I was consciously setting the value to "nothing", and we branch through the expectedFields setting logic with that.
... I could have just used undefined, but I felt null was a clearer representation of what I was doing.
This way we keep the graphql concept of "i tried to look up a type and it wasn't defined" and the stack concept of "am I in an inlinefragment" logic separate.
lopert
force-pushed
the
lopert.extra-fixtures
branch
from
c612396 to
e1f1712
Compare
lopert
force-pushed
the
lopert.extra-fixtures
branch
from
e1f1712 to
a368c19
Compare
saga-dasgupta
left a comment
saga-dasgupta
left a comment
There was a problem hiding this comment.
Was able to run through this code and ensure it works in detecting extra fields using some example fixtures and input queries.
| for (const fixtureField of fixtureFields) { | ||
| if (!expectedForThisObject.has(fixtureField)) { | ||
| errors.push( | ||
| `Extra field "${fixtureField}" found in fixture data not in query`, |
There was a problem hiding this comment.
Should we add a path to the bad field here to make it easier for people to debug? This might when the field is something common like id or title and the fixture is huge and they don't where the extra field is.
|
|
||
| // Check each field in the fixture object | ||
| for (const fixtureField of fixtureFields) { | ||
| if (!expectedForThisObject.has(fixtureField)) { |
There was a problem hiding this comment.
We might wanna exclude __typename here.
lopert
force-pushed
the
lopert.extra-fixtures
branch
from
0e07755 to
edc262b
Compare
adampetro
left a comment
adampetro
left a comment
There was a problem hiding this comment.
I think there are a few lint errors to fix but otherwise LGTM
3 participants
Redo of #27 on the latest.
Detects extra fields by keeping track of expectedFields (aka the ones we've visited) in a new stack, parallel to the existing valueStack. Then when leaving a selection set, compare what's available in the fixture to the visited field names to find extras.