Skip to content

Conversation

@sebhub
Copy link
Member

@sebhub sebhub commented Oct 4, 2019

The ItemValidator._get_issues_tree() did write completely wrong links
back to the item, since the item variable was re-used as a loop
variable (now: parent). This bug did not manifest badly since
Item.set_links() had no auto_save decorator. Remove the superfluous
Item.set_links() and use the setter instead.

The ItemValidator._get_issues_tree() did write completely wrong links
back to the item, since the item variable was re-used as a loop
variable (now: parent).  This bug did not manifest badly since
Item.set_links() had no auto_save decorator.  Remove the superfluous
Item.set_links() and use the setter instead.
@sebhub sebhub requested a review from stanislaw October 4, 2019 18:04
@stanislaw
Copy link
Member

stanislaw commented Oct 4, 2019

@sebhub thanks for catching this. This behavior of 1) finding issues and also 2) reformatting the item in the same method was something I wanted to raise later and refactor as a violation of SRP but I also managed to introduce a bug there 😞

I want to provide a test that ensures your change is included in the test harness. I will do it today or tomorrow max.

ItemValidator: 2 tests for doorstop.settings.REFORMAT == True and == False
@sebhub
Copy link
Member Author

sebhub commented Oct 15, 2019

I think the drop in code coverage due to the patch is harmless. The patch essentially removes a bit of code that was covered 100%. Since the overall coverage is below 100%, removing code with 100% code coverage drops the overall coverage since the ration between covered and uncovered code changes in favour of uncovered code.

@stanislaw
Copy link
Member

I am not sure why you think that the coverage is the same. One of the tests I have written does @patch('doorstop.settings.REFORMAT', True) which makes the following branches to be executed (was not the case before).

        # Delay item save if reformatting 
        if settings.REFORMAT: 
            item.auto = False 

and

     if settings.REFORMAT: 
            log.debug("reformatting item %s...", item) 
            item.save() 

I guess, this is what makes CI to be green now.

@stanislaw stanislaw merged commit 74e8c61 into doorstop-dev:develop Oct 15, 2019
@stanislaw
Copy link
Member

But it is true that it is not clear, why the coverage in your changeset gets to be a bit less than 95%.

@sebhub
Copy link
Member Author

sebhub commented Oct 15, 2019

Sorry for the confusion, my comment about the coverage was intended for my patch only and not the new tests. Lets say your code base has 10 lines, 5 lines are covered by tests, then your code coverage is 5 / (5 + 5) = 50%. If you remove one line of code which is covered but superfluous, then your coverage is 4 / (4 + 5) = 44%. I think this is what happened with my patch.

@sebhub sebhub deleted the fix-item-validation branch October 15, 2019 19:19
@stanislaw
Copy link
Member

Yes, your explanation makes sense. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants