we used to add annotation above a variable, in this case:

@Shared
private def forecastingServiceStub

yeah I usually only keep them on one line for things like tests, which potentially need to stub/declare a bunch of simple fields. This way they only get one line each and so don't clog up the file too much but I'm happy to split them up so that we stay consistent.

there seems to be too much indentation (between setupSpec and the setup methods)

yes I think this is because I mixed tabs and spaces. From what I can see our code standard is 4 spaces so I'll reform the file. I used tabs for years so it's going to take a minute to shake that convention out of me hehe.

(As a side note, we should really have a standardized prefs file that we can import to Intellij so that all our setups stay in sync.)

the indentation seems to be too big - we usually use 2 for tab.

I think for groovy files we have 4 (at least that's what I have tab size: 4, indent: 4, Continuation indent 8), 2 for tab we have for js

as for if/else statements, we used to write them with the { }, in this case it'd be:

if (shouldFindLocation) {
  json.data.location != null
} else {
  json.data.location == null
}

btw I have a question what the json.data.location != null does, as it would return a boolean (it compares the json.data.location with the null)?

yes that makes sense. I'll fix.

Actually you're right. Spock treats anything in the "then:" block that returns a boolean the same as if you prefixed it with "assert". If the condition returns false, it'd fail the test. BUT if the statement is nested in an if or else, then you need to explicitly provide the "assert" for it to trigger. I'll fix

wasn't it supposed to initialize the json.data.demand with an empty Map? This line just checks if json.data.demand is equal to the empty map.

It's supposed to be an == because I'm testing that it returned the data from the mocking that I did in forecastingServiceStub.getDemand(_, _, _) >> [:]. I suppose I could mock something more interesting there though so it's more obvious that it's actually returning something.

@ewaterman I haven't written many tests, so I didn't notice it is a shortcut of the assert. If it is the case, then I'm fine with that.

nitpicky:

+ Please, use types always where it is possible

The indents here look really odd, I am not sure if this is because of the GitHub or if it is broken in the code

same as above. I'll reformat

We are using braces in if body

I am not sure if this is correctly compared. I mean if we are comparing the content of the array or just the references between those two objects 🤔

This test is just verifying that sorting works so as long as the correct Person objects are in the correct places in the list we consider this a pass. Other tests verify the actual fields themselves. Changing the order in the expected array to something like [person1, person3, person2, person4] correctly fails the test so I feel confident this is working.

types

I agree. I didn't change this one because I wanted to show the minimum number of changes needed to refactor the test but since I'm changing that line anyways, yes I'll update it.

actually, these defs are all over the file and so it'd feel weird to just refactor this one case and not go through and refactor all of the tests, many of which are unchanged, so I'm just going to leave it as is for now.

I am fine with that 😄