Fix sectionEnded() not popping corresponding section from sectionStarted() in case where error was thrown in some section. #1651
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DanielPBak
changed the title
|
I wasn't aware of the existence of the approval tests and I see the issue. Will find a solution, and add an approval test which fails against the behavior in master. |
|
While the tracking code probably could use a clean-up, it is fairly battle-tested, and as you've found out yourself, the behaviour required can be subtle. OTOH, if you can improve things, it would be helpful. |
Codecov Report
@@ Coverage Diff @@
## master #1651 +/- ##
==========================================
- Coverage 81.05% 80.72% -0.33%
==========================================
Files 124 124
Lines 3478 3476 -2
==========================================
- Hits 2819 2806 -13
- Misses 659 670 +11 |
Codecov Report
@@ Coverage Diff @@
## master #1651 +/- ##
=========================================
- Coverage 83.67% 80.7% -2.97%
=========================================
Files 123 124 +1
Lines 3380 3476 +96
=========================================
- Hits 2828 2805 -23
- Misses 552 671 +119 |
Editied approval tests to expect correct behavior.
…ng correctly on my machine.
… could possibly pop different data than was pushed. This is because sectionEndedEarly() would push the failing section onto another stack (m_unfinishedSections), and then the next sectionEnded() would close the hanging sectionStarted(). Then the m_unfinishedSections would be popped, filling up the remaining hanging sections. This caused two issues: One: any reporter relying on SectionEnd() having accurate section data was broken. SectionEnd() would present different data than SectionStarted(). This caused an issue for the junit reporter, which relies on that data matching to manage the parralel m_sectionStack. However, this also applied to other reporters; having the xml reporter write the "name" attribute to the OverallResults written in sectionEnded revealed that it was not matching the section. Two: I noticed where tests containing failing sections would pass through again, but not hit any nodes. Since a failing section would not be popped, it would remain on the stack, and this causes the test runner to believe that the test requires another pass because there are unresolved open sections (when all sections were actually closed).
DanielPBak
force-pushed
the
section-OOO-fix
branch
from
010393f to
1323697
Compare
Closed
|
I don't believe that it only requires a clean-up - this is legitimately broken behavior. But it certainly requires a solution that doesn't break significantly more than it fixes :P I've submitted #1654 which is a separate PR containing only the approval tests which reveal the problem. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In a failing test, the sectionend() corresponding to a sectionstart() could possibly pop different data than was pushed. This is because sectionEndedEarly() would push the failing section onto another stack (m_unfinishedSections), and then the next sectionEnded() would close the hanging sectionStarted(). Then the m_unfinishedSections would be popped, filling up the remaining hanging sections.
This caused two issues:
One: any reporter relying on SectionEnd() having accurate section data was broken. SectionEnd() would present different data than SectionStarted(). This caused an issue for the junit reporter, which relies on that data matching to manage the parralel m_sectionStack
However, this also applied to other reporters; having the xml reporter write the "name" attribute to the OverallResults written in sectionEnded revealed that it was not matching the section.
Two: I noticed where tests containing failing sections would pass through again, but not hit any nodes. Since a failing section would not be popped, it would remain on the stack, and this causes the test runner to believe that the test requires another pass because there are unresolved open sections (when all sections were actually closed).
This PR fixes both of these problems and does not seem to have any down side. Is there some edge case where it is important to pop the stack out of order? To me this looks like legacy code that is no longer useful.
The change I made was just to always call sectionEnded() and never sectionEndedEarly().
In my opinion, this needs a larger workaround to avoid introducing similar bugs. sectionEnded() shouldn't consume anything and should automatically pop the top.
Sorry about the verbose description.
GitHub Issues
Closes #1650
Closes #417
It also looks related to #1210