-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Fixes issue #2326 #2327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes issue #2326 #2327
Conversation
@microsoft-github-policy-service agree |
Discovered an additional defect in scope of issue #2326 , involving closing parenthesis immediately followed by a decimal place. For example:
Description of the changes:
How changes were validated:
Sample testing steps:1.) Open the calculator app, and from the hamburger menu, select "Scientific". 3.) Input
4.) Click the Additional Details:
Note: Similar to my previous comment (here), there are currently up to 7 UI test failures that are unrelated to this PR, and have already either been failing or intermittently failing prior to this pull request. |
Thank you for initiating this PR to fix the bug, @Sinclo! It looks like our CI run was expired and it's blocking us to complete this PR. |
Thank you @tian-lt, @hanzhang54, and @guominrui 👍🏾 |
Fixes issue #2326
Description of the changes:
CCalcEngine::ProcessCommandWorker
method, added a condition so thatCheckAndAddLastBinOpToHistory()
is only triggered if the last command is not a closing parenthesis. This addresses the issue where the calculator would prematurely insert a record into the "History" tab, when typing)
followed by a number.IDC_EQU
case as wellHow changes were validated:
CalculatorManagerTest::CalculatorManagerTestScientificParenthesis()
unit test, to account for scenarios involving any numbers input after closing parenthesis.SmokeTest_CloseParenthesis()
, to verify the UI behavior of my changes, and ensuring that the calculator and history results are displayed correctly.Sample testing steps (after code changes):
1.) Open the calculator app, and from the hamburger menu, select "Scientific".

2.) Input
(
8
)
.3.) Input

2
(without any operator being specified, including the multiplication operator). Observe how the calculator results are displayed as(8) x 2
, and that there are no "History" tab results yet.4.) Click the

=
button. Observe the calculator results are correctly displayed as(8) x 2 = 16
, and that there is a "History" tab result of the same as wellAdditional Details:
Aot_EnterExitKeepOnTop
andAoT_Tootip
) which fail intermittently before & after code changes, due to flakiness. These tests have nothing to do with my changes.(8)2=
into the calculator, the output would be displayed with the operator (for example, as(8) x 2 =
). I purposely did this to remain consistent with how the calculator displays results from other use cases today.SmokeTest_CloseParenthesis()
UI test that I added, I made this separate from the existingSmokeTest_Parentheses()
test, as the latter seemed to be designed to actually input thex
operator in the tests (which is different from the use cases described in issue #2326 and #1498, which do not input thex
operator between parenthesis). For this reason, I madeSmokeTest_CloseParenthesis()
a separate test, with a focus on the "CloseParenthesis" use cases for now.SmokeTest_OpenParenthesis()
test as well (with a focus on the "OpenParenthesis" use cases from #1498), but chose not to perform that as part of this issue to avoid scope creep. I would be happy to submit another issue and work on this separately, if desired.SmokeTest_CloseParenthesis
and set it as "Priority 0" for now. However, I'm not fully familiar with how test priority or the testing strategy is structured, so please feel free to provide feedback if the priority and/or testing name should be updated, if it should be redesigned or consolidated with another test, etc.