Should I assume all the type checking are not ignored?

[INFCode]

TL;DR: If there are some branches of if statements that can only be covered by tests using the #type ignore annotation, should I still add a test to cover them, or should I remove the checking?

---

Hi, I am new to Oppia and am currently working on issue #14219 for core.controllers.domain_objects_validator_test.

I noticed there are 3 if statements (see below) whose false branches are not covered. However, I find that they are checking the existence of fields in an instance of a subtype of TypedDict. So the variable should always contain these fields as long as the type checking is passed. Therefore these branches are not possible to be covered without ignoring the type checking. But I am not sure if this is really a good idea. As mentioned in the backend test guidance wiki, we should "Test the interface, not the implementation". And According to the interface, these fields should definitely be there. Then it seems not reasonable to test what happens if the input does not have these fields.

So should I 1. just remove the three if statements as they will never fail, or 2. use #type ignore annotation and test them anyway.

Here is the related code:

def validate_change_dict_for_blog_post(
    change_dict: blog_services.BlogPostChangeDict
) -> blog_services.BlogPostChangeDict:
    """ Docstring omitted
    """
    if 'title' in change_dict:  # 1
        blog_domain.BlogPost.require_valid_title(
            change_dict['title'], True)
    if 'thumbnail_filename' in change_dict: # 2
        blog_domain.BlogPost.require_valid_thumbnail_filename(
            change_dict['thumbnail_filename'])
    if 'tags' in change_dict: # 3
        blog_domain.BlogPost.require_valid_tags(
            change_dict['tags'], False)
        # rest of the function is omitted

The three commented if statements are checking if certain fields are present in the change_dict, but according to the type annotation, it should be an instance of blog_services.BlogPostChangeDict, which is defined as follows:

class BlogPostChangeDict(TypedDict):
    """Dictionary representing the change_dict for BlogPost domain object."""
    title: str
    content: str
    tags: List[str]
    thumbnail_filename: str

So the given fields are required to exist by the type system.

[U8NWXD]

In this case you should write tests to cover those if blocks that can only be reached by providing "wrong" types. In those tests, you will need to tell mypy to ignore the type clashes.

Once we have the backend code fully typed and are confident that we can rely on the type checking, we can ignore these if checks for incorrect types. In the mean time, we need to keep the if checks and have tests to ensure they're working.

[INFCode]

Thank you for the advice! I'll work on that!

[INFCode]

Oops, I just noticed another question. What should be the expected behavior then? Should my tests expect validate_change_dict_for_blog_post to simply ignore the missing fields silently as it is the current behavior of the function?

[U8NWXD]

Hmm I generally prefer that we raise errors noisily rather than silently ignoring them, but this is really up to the @oppia/data-and-stability-reviewers team. @kevintab95 wdyt?

[seanlip]

Yes, always raise errors noisily. Thanks!

[INFCode]

It seems like I will have to modify the function. However, I am uncertain whether this should be incorporated in the same PR since the objective of it is to solely enhance the coverage. Moreover, are there any specific preferences or guidelines that I should follow while writing the exception message, or can I simply draft it based on what I consider to be informative?

[seanlip]

It's fine to modify the function in your "increase coverage" PR. Sometimes adding tests will require minor refactoring of the code (e.g. to cover missed cases in the logic), and that's not an issue.

It's fine to write the exception message as you see fit. Having it be informative is a good heuristic to use.

[INFCode]

Raising errors noisily seems to break some other tests, e.g. test_publishing_unpublishing_blog_post in core.controllers.blog_dashboard_test.py fails after the change (See the current backend test status of #18042). In this test, an empty change_dict is directly sent when attempting to unpublish a blog post, with the assumption that the request will succeed. I'm not familiar with Oppia's conventions, so I'd like to inquire whether sending empty change_dicts is a standard practice in Oppia (in which case, I should mark the fields of BlogPostChangeDict as optional instead of raising an error), or whether the test is making an incorrect assumption (in which case, I'll need to modify this test instead).

[seanlip]

@INFCode Thanks for catching this! The test is making an incorrect assumption and should be modified. You can also add a test with an invalid change dict for this case to ensure it results in an error.

(Btw, this is a case where the updates you're making caught a bug in the tests/code, so that's something to celebrate!)