Refactor tests for property queries and aggregate queries #190
Conversation
kevinwcyu
requested review from
iwysiu and
sarahzinger
and removed request for
a team
|
Levitate is-compatible report: 🔍 Resolving @grafana/data@latest... 🔬 Checking compatibility between ./src/module.ts and @grafana/data@9.5.2... 🔬 Checking compatibility between ./src/module.ts and @grafana/ui@9.5.2... 🔬 Checking compatibility between ./src/module.ts and @grafana/runtime@9.5.2... 🔬 Checking compatibility between ./src/module.ts and @grafana/e2e-selectors@9.5.2... ✔️ ./src/module.ts appears to be compatible with @grafana/data,@grafana/ui,@grafana/runtime,@grafana/e2e-selectors |
|
Yowza! Nice job, looked like a lot of work. Pretty overwhelming to read still but I'm not sure what could make an end-to-end style test more readable. Another idea I have (future?) is to have smaller tests at each layer. The backend in this repo is a little bit "too much" but at least there is a method to the madness, so there are some distinct layers... I hope, though, that your change will make some debugging easier one day. Something I am missing is where the golden files are still getting modified. I see they were removed in these files you modified, but do you think there was some value to having the large sets of data? (I don't actually know if there were large sets in these tests.) And also were there any files we are no longer using now that we pulled some input into the test files?
I think we should make this the focus of this PR if it's a change in the behavior, or at least add it as one of the main things the PR does. What do you think? |
There was a problem hiding this comment.
This looks cool to me but I'm also not very confident in my golang skills yet, so I defer to @fridgepoet on this one! Thanks for taking this on!
| @@ -28,7 +28,7 @@ func (rp *SitewiseResources) Asset(ctx context.Context, assetId string) (*iotsit | |||
| } | |||
|
|
|||
| func (rp *SitewiseResources) Property(ctx context.Context, assetId string, propertyId string, propertyAlias string) (*iotsitewise.DescribeAssetPropertyOutput, error) { | |||
| if propertyAlias != "" { | |||
There was a problem hiding this comment.
Might be good to separate out changes to the codebase from refactoring tests wdyt?
| *entries.AggregateTypes[0] == "SUM" | ||
| }), | ||
| mock.Anything, | ||
| mock.Anything, |
There was a problem hiding this comment.
do you think it would be easier to read if we named some of these inputs? I'm not sure if this is stylistically weird to do with go but I guess it feels like it'd be easier to follow this test if we were like:
mockSw.On(
"BatchGetAssetPropertyAggregatesPageAggregation",
mockRequestContext,
mockInput,
mockPages,
mockMaxResults,
)
wdyt? Maybe I'm being silly/nitpicky?
There was a problem hiding this comment.
These are just really huge tests since they're at the highest level (feels a bit end to end); I cannot yet find the ideal balance between something "digestable" and something where the reader doesn't get re-directed all the time to find the underlying values.
It's my opinion that by not extracting the inputs into variables, we make it easier to connect the query input to what's being passed to some of the methods, and so on down the chain.
I think this test remains difficult to read, but mostly because is testing a very long chain of external API calls. Some ideas: maybe duplicate this test to only make assertions on one method at a time (so we can just use mock.Anything) or we could separate the tests and test at lower levels.
I think this test a reasonable improvement over the previous input which was read from very far away files and sometimes quite lengthy, and an output which was also written to some file far away. I wouldn't be surprised to see further improvements iteratively as we dive into this repo more.
Anyway, I will ultimately support whatever Kevin thinks is best!
There was a problem hiding this comment.
i think i'll leave it as is for now and we can iterate on further improvements later.
i think keeping it as mock.Anything has the advantage of being more clear since it's what's provided by the mocking library and also when I hover over it I see the following so i can see a particular argument isn't important for the given test case.
const mock.Anything untyped string = "mock.Anything"
Anything is used in Diff and Assert when the argument being tested shouldn't be taken into consideration.
also a more minor reason, if we ever modify the arguments to these functions, we'd need to check every single place to make sure name still makes sense.
There was a problem hiding this comment.
For my 2 cents on this:
I'm ok with it as is (the other variables aren't the point of the test and this communicates that fairly well), but if we wanted it to be a little clearer to read, we could add comments next to each of them or a comment of the function signature above the mock.
Though as a note on the minor reason, I do think that changing the arguments of an exported functions should be a pain, since we should really avoid doing it. However, I also get that commenting the arguments doesn't actually stop us from changing things but adds the possibility of getting out of sync.
| } | ||
|
|
||
| mockSw.AssertExpectations(t) |
There was a problem hiding this comment.
sorry for the very basic questions on my part but like what does this assert?
There was a problem hiding this comment.
It makes sure that all the all the declared methods ("mockSw.On("...")") were called.
Sometimes we declare those mocks but the test doesn't even use them which can be misleading. It's kind of an indirect way to assert some paths/coverage, I guess.
I think some people might put these in everywhere, but I'm not sure it's always worth it. The reason I suggested it in these tests is that they're kind of "end to end," very long, complex, repetitive, and there are many to re-do. While pairing there were a few times we noticed that a test was written incorrectly and not testing what we thought.
kevinwcyu
changed the base branch from
main
to
kevinwcyu/call-describe-asset-property
| *entries.AggregateTypes[0] == "SUM" | ||
| }), | ||
| mock.Anything, | ||
| mock.Anything, |
There was a problem hiding this comment.
For my 2 cents on this:
I'm ok with it as is (the other variables aren't the point of the test and this communicates that fairly well), but if we wanted it to be a little clearer to read, we could add comments next to each of them or a comment of the function signature above the mock.
Though as a note on the minor reason, I do think that changing the arguments of an exported functions should be a pain, since we should really avoid doing it. However, I also get that commenting the arguments doesn't actually stop us from changing things but adds the possibility of getting out of sync.
Refactors the test for property value and property value aggregate queries.
Fixes: #188
Changes are based on comments from #179 (comment).
Changes made:
Notes: