Skip to content

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

Merged
merged 2 commits into from
Jul 2, 2025
Merged

Fixes issue #2326 #2327

merged 2 commits into from
Jul 2, 2025

Conversation

Sinclo
Copy link
Contributor

@Sinclo Sinclo commented May 5, 2025

Fixes issue #2326

Description of the changes:

  • In the CCalcEngine::ProcessCommandWorker method, added a condition so that CheckAndAddLastBinOpToHistory() 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.
  • Added an additional condition for closing parenthesis, to ultimately treat this implicitly as multiplication, and to maintain this behavior within calculator and history results.
  • Minor update of the IDC_EQU case as well

How changes were validated:

  • Ran several manual tests, and verified that the calculator results and history results are displayed correctly
  • Updated the existing CalculatorManagerTest::CalculatorManagerTestScientificParenthesis() unit test, to account for scenarios involving any numbers input after closing parenthesis.
  • Added a new UI automation test called SmokeTest_CloseParenthesis(), to verify the UI behavior of my changes, and ensuring that the calculator and history results are displayed correctly.
  • Ran all unit tests and UI tests - before and after code changes - to ensure that my solution did not introduce other defects in the process.
  • Unit test results before code changes:
    CalculatorUnitTestResults_BeforeCodeChange
  • UI test results before code changes:
    CalculatorUITestResults_BeforeCodeChange
  • Unit test results after code changes:
    CalculatorUnitTestResults_AfterCodeChange
  • UI test results after code changes:
    CalculatorUITestResults_AfterCodeChange

Sample testing steps (after code changes):

1.) Open the calculator app, and from the hamburger menu, select "Scientific".
2.) Input ( 8 ).
Issue1_Step2_AfterCodeChange

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.
Issue1_Step3_AfterCodeChange

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 well
Issue1_Step4_AfterCodeChange

Additional Details:

  • There are 5 existing UI tests that fail before & after my code changes. Additionally, there are also a couple additional UI tests as well (called Aot_EnterExitKeepOnTop and AoT_Tootip) which fail intermittently before & after code changes, due to flakiness. These tests have nothing to do with my changes.
  • Though I originally discovered this issue by accident while looking into issue #1678, the solution in this ticket does not address that issue at all. In other words, if you were to input (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.
  • Regarding the new SmokeTest_CloseParenthesis() UI test that I added, I made this separate from the existing SmokeTest_Parentheses() test, as the latter seemed to be designed to actually input the x operator in the tests (which is different from the use cases described in issue #2326 and #1498, which do not input the x operator between parenthesis). For this reason, I made SmokeTest_CloseParenthesis() a separate test, with a focus on the "CloseParenthesis" use cases for now.
  • Note: I also feel it could be valuable to have a separate 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.
  • Note: For the sake of consistency, I named my separate UI test as 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.

@Sinclo
Copy link
Contributor Author

Sinclo commented May 5, 2025

@microsoft-github-policy-service agree

@Sinclo
Copy link
Contributor Author

Sinclo commented May 18, 2025

Discovered an additional defect in scope of issue #2326 , involving closing parenthesis immediately followed by a decimal place.

For example:

  • If inputting (8)0.5= into the calculator, this use case worked fine, and correctly returned (8) x 0.5 = 4 within the calculator results and the history results.
  • However, if inputting (8).5= into the calculator (without explicitly specifying the 0 before the decimal), this wasn't being handled by the calculator correctly. Instead, both the calculator results and history results were incorrectly returning (8) = 0.5.
  • I've fixed this use case in my latest commit, so that this now correctly returns (8) x 0.5 = 4 in the calculator and history results. Additionally, I've updated the existing unit and UI tests to cover this use case as well.

Description of the changes:

  • Added an additional condition for closing parenthesis under the IDC_PNT case.

How changes were validated:

  • Ran several manual tests, and verified that the calculator results and history results are displayed correctly
  • Updated the CalculatorManagerTest::CalculatorManagerTestScientificParenthesis() and SmokeTest_CloseParenthesis() tests, to account for this additional use case
  • Ran all unit tests and UI tests again, to ensure that my solution did not introduce new issues in the process.

Sample testing steps:

1.) Open the calculator app, and from the hamburger menu, select "Scientific".
2.) Input ( 8 ).
051825_Issue1_Step2

3.) Input . 5 (without a multiplication sign or 0 before the decimal place).

  • Note: In this use case, once you click the . (decimal) value, the calculator will automatically add in the multiply operator and the 0 value on its own.

051825_Issue1_Step3

4.) Click the = button. Observe that both the calculator results and history results are correctly displayed as (8) x 0.5 = 4
051825_Issue1_Step4

Additional Details:

  • I updated the CalculatorManagerTest::CalculatorManagerTestScientificParenthesis() and SmokeTest_CloseParenthesis() tests from this PR, to account for this latest defect
  • I ran all unit tests and UI tests again as well, to ensure my latest changes did not break any other use cases.

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.

@tian-lt tian-lt requested review from hanzhang54 and guominrui July 1, 2025 03:08
@tian-lt
Copy link
Contributor

tian-lt commented Jul 1, 2025

Thank you for initiating this PR to fix the bug, @Sinclo!
The changes look good to us, and we'd like to have them checked in to the codebase.

It looks like our CI run was expired and it's blocking us to complete this PR.
Please allow me to try closing and then reopening this PR to see if the PR Check can pass.

@tian-lt tian-lt closed this Jul 1, 2025
@tian-lt tian-lt reopened this Jul 1, 2025
@tian-lt tian-lt merged commit 0e6d669 into microsoft:main Jul 2, 2025
5 checks passed
@Sinclo
Copy link
Contributor Author

Sinclo commented Jul 30, 2025

Thank you @tian-lt, @hanzhang54, and @guominrui 👍🏾

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.

4 participants