Fundamental test coverage of vgrid module reusing existing structure.

[jonasbardino]

Fundamental test coverage of vgrid module reusing existing structure.

Some tests are disabled on purpose because changes are needed in the tested module. Marked as follow-up.

[albu-diku]

When we have a situation where a single test function is actually testing multiple things like this to me that's a hint to each of these ought to be their own test function.

Moreover, there seem to be two different "starting" states for these tests - there is a "with no existing vgrid" and "with an existing vgrid", and likely different needs in terms of setup code for them. With a lens like that, it suggests two test case blocks:

class TestCase_no_vgrid:
pass

class TestCase_with_existing_vgrid:
pass

with the tests shared between the two, and ech of the "Test 1, Test 2, ..." things in here being much more naturally separate functions in the latter testcase. Note you get one other benefit: you can then tailor before_each to both of the situations. In the "with existing vgrid" case you can place the setup code for provisioning a vgrid in before_each and then that details doesn't need to exist in each test function.

[jonasbardino]

Agreed, and I already started splitting similar constructs in other PRs now as you may have recently seen. I have continued here but there's probably room for more. Yet, I think there's no point in delaying this one much further, so I lean towards merging with the latest updates and keeping it in mind for future unit tests or refactoring in a follow-up PR.

[albu-diku]

Yah - since that comment was written towards the end of October last year we collectively definitely been more consistently splitting things etc :) That's for having revisited it, even if a little, here.

Aside, I'm as ever on the side of lets get the tests in, but much like being cautious against small workarounds that have that nasty tendency to persist (and for that reason we are rightly way of) I feel the same about tests - trying to maintain that balance etc.

[jonasbardino]

Thanks, and yes while this is a work in progress I believe we're on the right track. I'll proceed with merging and see if the recent merge of #427 gives rise to obvious test additions as follow-up.

[albu-diku]

I think this is a very important area to cover and it will be a boon to have coverage of this in tree.

I've left a comment with suggestions about how I think the tests could be restructured a little for what I think will be a big aid to comprehensibility here: https://github.com/ucphhpc/migrid-sync/pull/371/files#r2472789903. I'm not going to insist hence delivery as a comment rather than requesting changes, but I do feel it worthwhile if possible.

[jonasbardino]

I think this is a very important area to cover and it will be a boon to have coverage of this in tree.

I've left a comment with suggestions about how I think the tests could be restructured a little for what I think will be a big aid to comprehensibility here: https://github.com/ucphhpc/migrid-sync/pull/371/files#r2472789903. I'm not going to insist hence delivery as a comment rather than requesting changes, but I do feel it worthwhile if possible.

Thanks for the review. I've updated and requested another review to also ask for approval before merging.

[albu-diku]

I don't feel quite as confident in terms of having detailed knowledge of the functionality being tested, and as such a word of caution that I can't comment on whether we have exercised all the behaviours we should - but the tests themselves here look good at this point, so approving that aspect.