Tests: relocate 'ingredient_groups' from mandatory-tests to optional-tests.

[jayaddison]

...and, at the same time, remove the ingredient_groups key from any data-driven test JSON expectation files for scrapers that don't implement custom retrieval of grouped ingredients.

This requires a small change to the test suite so that test expectations are not created when not required.

Resolves #1181.

[jknndy]

This a great cleanup!

What do you think about removing the ingredient_group JSON key for the non-ingredient_group test of the sites that do contain the functionality? All sites that contain grouping have 2 test cases one covering the grouping functionality & another not, this could be another substantial clean up but would need to be done manually.

Alternatively, this currently passes if ingredient_groups is removed regardless if the scraper class contains an ingredient_group override. Using AbuelasCounter as an example you can remove the ingredient_groups key from *_2.json and the tests will still pass. I wonder if it would be possible to run a check the scraper class to see if an override exists and if so expect the JSON key. This would remove the possibility to implement the first idea unless we were to check the output to see if it is producing a single set of ingredients with a purpose of null and not expect the key in the JSON.

[jayaddison]

Both good ideas, yep! Just to check I understand them correctly:

• Remove ingredient_groups from test data in cases where an alternate .json file already exists that covers the non-default / basic single-group case.
• When a method has a custom implementation in a scraper, ensure that a JSON key exists with the same name (approximately similar to treating it as a mandatory-test field).

[jayaddison]

(and it took a few moments, but I understand what you mean about the first idea being more difficult to do if we implement the second idea)

[jayaddison]

This would remove the possibility to implement the first idea unless we were to check the output to see if it is producing a single set of ingredients with a purpose of null and not expect the key in the JSON.

I'd like to avoid this route if possible - our tests aren't strictly isolated from the library code at the moment, so there can be some behavioural interference between test code and library code -- but generally I think we should write tests that are not allowed to gain much (or ideally any) information about the corresponding scraper and its output before deciding what to assert on.

[jknndy]

• Remove ingredient_groups from test data in cases where an alternate .json file already exists that covers the non-default / basic single-group case.

yep, exactly!

• When a method has a custom implementation in a scraper, ensure that a JSON key exists with the same name (approximately similar to treating it as a mandatory-test field).

& yep again! I was thinking just about ingredient_groups but this could be implemented across all coverages.

I'd like to avoid this route if possible - our tests aren't strictly isolated from the library code at the moment, so there can be some behavioural interference between test code and library code -- but generally I think we should write tests that are not allowed to gain much (or ideally any) information about the corresponding scraper and its output before deciding what to assert on.

I agree, this is definitely a last resort kind of idea.

[jayaddison]

Ok, so, ignoring the last-resort situation for a moment, I think what we're talking about is whether test coverage should be enforced for overridden methods (case 2), or whether it is optional and we can omit the expectations if so (case 1).

Arguably test coverage reporting tools could already do the check from case 2 for us -- they could tell us that an overridden method hasn't been covered (coverage run -m unittest && coverage html for example, to generate an HTML test coverage report). Perhaps we could configure our continuous integration to let us know about methods without test coverage?

If we did use continuous integration to detect this using coverage tools, that still leaves a question about whether to remove the expectations in cases where we have multiple tests.

Whatever the scraper -- even for ones that don't override ingredient_groups -- we continue to check that the results are retrieval-consistent with ingredients.

[jknndy]

Ok, so, ignoring the last-resort situation for a moment, I think what we're talking about is whether test coverage should be enforced for overridden methods (case 2), or whether it is optional and we can omit the expectations if so (case 1).

After thinking about it more, I’m leaning towards case 1. Removing the test data from non-ingredient_groups tests offers the most cleanup. Additionally, if the developer went through the process of working out the ingredient_groups scraping they are likely to include test coverage for it. We can always track this approach for a while and revisit if it causes confusion.

note: are there any doc updates required as a result of this change?

Arguably test coverage reporting tools could already do the check from case 2 for us -- they could tell us that an overridden method hasn't been covered (coverage run -m unittest && coverage html for example, to generate an HTML test coverage report).

Thanks for the tip! Had no idea this existed, lots interesting info here.

Perhaps we could configure our continuous integration to let us know about methods without test coverage?

This sounds worth looking into but that may be a separate discussion. I've written code in the past to generate the missing coverages that could possibly be repurposed to do this.. i'll have to look around to see if i hung on to it.

[jayaddison]

Ok, so, ignoring the last-resort situation for a moment, I think what we're talking about is whether test coverage should be enforced for overridden methods (case 2), or whether it is optional and we can omit the expectations if so (case 1).

After thinking about it more, I’m leaning towards case 1. Removing the test data from non-ingredient_groups tests offers the most cleanup. Additionally, if the developer went through the process of working out the ingredient_groups scraping they are likely to include test coverage for it. We can always track this approach for a while and revisit if it causes confusion.

That makes sense to me - I'll go ahead with cleaning up those files.

note: are there any doc updates required as a result of this change?

Mm, good question - it seems quite likely. I'll check that too.

[jayaddison]

Ok, I think that's more-or-less ready now; glad for any more suggestions/review.

[jknndy]

Just one last thing, in the docs/in-depth-guide-ingredient-groups.md i think some changes are needed as well as the tests no longer automatically populate the ingredient_groups key

In addition to the usual tests a scraper requires, the tests also needs to check the groups and the ingredients in each group are correct for the recipe. For the test cases where there are no ingredient groups, this should check for a single IngredientGroup object with purpose=None and ingredients set to the output from the scraper's ingredients() function. For the test case with ingredient groups, the output should match the groups in the recipe.

Each test case will automatically inherit a test that checks to make sure the same ingredients are found in .ingredients() and in the groups returned from .ingredient_groups(), so there is no need to write this test in the scraper test cases.

Otherwise, looks good to go to me!

[jayaddison]

Good catch, thank you!

[jknndy]

Looks good to me!