Skip to content
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

fixed decimal usability in converter section #1896

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

DeclanGazil02
Copy link

Fixes #1832.

After selecting a currency that does not handle fractional units, the decimal is turned off. This is the correct functionality. However, when a user goes to change the category of conversion, say to length, then the decimal is still turned off despite needing to be on. The only way the decimal can be reenabled is to go back to the currency category and change the units to a currency that allows for fractional units.

Description of the changes:

  • I inserted the line "IsDecimalEnabled = true" inside the ResetCategory() method of the unit converter.
    In my thinking, the decimal should be reenabled upon category switching and will be disabled when a unit with non-fractional units is being used. All code styles were followed, and no major code changes occurred.

How changes were validated:

  • Rigorous manual testing. Swapping between categories and units to ensure the decimal is enabled when it needs to be.

@tian-lt
Copy link
Contributor

tian-lt commented Sep 26, 2022

Since we have UpdateIsDecimalEnabled() to take care of the property, consider using it instead of giving a direct value to IsDecimalEnabled?

Add @xuhongxu96 to help review on this :)

@DeclanGazil02
Copy link
Author

I corrected my code to use UpdateIsDecimalEnabled(), as @tian-lt suggested, however some necessary changes to that function were to be made as well. The function only updated the decimal when going back and forth between currency units, and if the category was not a currency, then it would simply return, keeping IsDecimalEnabled as is. I made the change to where if the calculator is not in the currency section, then IsDecimalEnabled = true. This is because the decimal can only be disabled when using the currency category, so upon leaving it should default to true.
I then forked the most recent version of the repository to bring my code up to date and ensure it would still function, and then readded my changes, as seen in the last commit.
I tested rigorously to ensure everything worked as is.
@xuhongxu96, I was told to add you to help review this :)
(also, I was a little uncertain about committing again after already making a pull request, and didn't know it would add to the same one. Sorry about the poor naming conventions on my recent commits)
Thank you for your time and help!

@xuhongxu96
Copy link
Contributor

xuhongxu96 commented Sep 28, 2022

Thanks! First of all, I think it is better to focus the PR on the bug fixing and not change the CI, package versions and etc.

We need to narrow down the cause of the test failure. Could you please try the test steps in CalculatorUITests.CurrencyConverterFunctionalTests.MouseInput_SwitchCurrencyWithLessFractionalDigitsAndEnterInput to see if it can be re-produced manually?

BTW, since your 1st commit succeeded building, I doubt the failure was caused by some newer packages you have updated in the later commits.

@DeclanGazil02
Copy link
Author

@xuhongxu96 I typed in 2.435 into the Test Fraction Digits currency type which produces a 0 in the Test No Fractional Digits type below, due to the conversion rate. I then selected the Test No Fractional Digits type below and was able to type 4 into it both times. However, I believe I have another issue because I reverted my altered code to the code that is in the main branch and it still failed the tests. Is it possible for me to close this commit, work on a new branch based on the main-branch code, and do another pull request once I have it working?
I'm sorry for the inconvenience, I am still new to this process. Thank you so much for working with me. I really want to make this contribution happen.

@xuhongxu96
Copy link
Contributor

@DeclanGazil02 No problem at all and let's make it merged!

Is it possible for me to close this commit, work on a new branch based on the main-branch code, and do another pull request once I have it working?

I think @tian-lt can provide more guidance. I think the discussions here are valuable, so let's continue using this PR (you can make it draft PR before it's ready). And to start new changes from the master branch, you can rebase/revert your current branch and force push the changes.

@tian-lt
Copy link
Contributor

tian-lt commented Sep 28, 2022

Thanks to @xuhongxu96 for the advice.

Hi @DeclanGazil02, it seems this PR contains a number of irrelevant changes. You may open new PRs to describe the motivations (with some simple words) and to split the changes into the ranges that are more comprehensible for reviewing.
You may also double check the failed UI test case CalculatorUITests.CurrencyConverterFunctionalTests.MouseInput_SwitchCurrencyWithLessFractionalDigitsAndEnterInput to make it clear if it is a regression or a flaky test. (You can re-run the failed tests multiple times to see if it's flaky)

Thanks again to your interests and your contribution and looking forward to seeing this PR gets completed.

@DeclanGazil02
Copy link
Author

@tian-lt I just re-downloaded the latest code from the main branch and all the tests in the CalculatorUITests are failing, all others are passing. It's throwing an exception stating System.Exception: The local WinAppDriver server has not been started. Is this something on my end?

@tian-lt
Copy link
Contributor

tian-lt commented Sep 28, 2022

Have you installed the WinAppDriver? It is a fundamental tool that UI test cases rely on.
Here is link to the latest (stable) version: https://github.com/microsoft/WinAppDriver/releases/tag/v1.2.1

And other points to pay attention to are:

  1. Pick a proper test settings to kick off the UI test runs. You can find them at here: https://github.com/microsoft/calculator/tree/main/src/CalculatorUITests
  2. Maximize the Calculator window to avoid some known (flaky) issues that WinAppDriver may hit.

@DeclanGazil02
Copy link
Author

I downloaded WinAppDriver @tian-lt, and now the only tests that are failing are due to System.ArgumentNullException: text cannot be null (Parameter 'text'). and OpenQA.Selenium.WebDriverException:.
I did not install the WinAppDriver NuGet package because I didn't know how. Is this where my issue is arising?
I was also having trouble picking a proper test setting as you said, what do I do with the files you linked?

@tian-lt
Copy link
Contributor

tian-lt commented Sep 28, 2022

If WinAppDriver is installed properly on your machine, all should be fine. No nuget package is needed.

About test settings, for this PR, CalculatorUITests.ci-internal.runsettings should be good to use.
To pick up the test settings in Visual Studio:
Main menu bar > Test > Configure Run Settings > Select Solution Wide runsettings file > (pick the file in the launched file picker).
image

@DeclanGazil02
Copy link
Author

@tian-lt now all tests are actually running. 10 tests are failing but due to wrong output. For example MouseInput_SelectCurrencyWithoutFractionalDigitEnterInputAndCheckTheFormat is expecting Japanese Yen but is actually outputting Jupiter Gas Money. The Japanese Yen is only on the live version of the calculator (already installed on my computer) and not the Dev version that I am using. Is there a way to swap between the two? I feel as if this is where my tests are going wrong. Thank you again for all your help, this is an incredibly valuable learning journey for me.

@DeclanGazil02
Copy link
Author

@tian-lt now all tests are actually running. 10 tests are failing but due to wrong output. For example MouseInput_SelectCurrencyWithoutFractionalDigitEnterInputAndCheckTheFormat is expecting Japanese Yen but is actually outputting Jupiter Gas Money. The Japanese Yen is only on the live version of the calculator (already installed on my computer) and not the Dev version that I am using. Is there a way to swap between the two? I feel as if this is where my tests are going wrong. Thank you again for all your help, this is an incredibly valuable learning journey for me.

I also updated my test settings as you mentioned. Thank you for the in-depth walk through!

@tian-lt
Copy link
Contributor

tian-lt commented Sep 30, 2022

10 tests are failing but due to wrong output. For example MouseInput_SelectCurrencyWithoutFractionalDigitEnterInputAndCheckTheFormat is expecting Japanese Yen but is actually outputting Jupiter Gas Money.

Jupiter Gas Money is from mocked currency data which is designed for tests. If you see some cases failed because they expected some real currency units, please try different runsettings to bypass the test cases. If you cannot make them pass on your machine, the easiest way to verify them is to trigger the CI pipeline via GitHub PR and look for the test cases you would like to verify in the test results to see if they passed the test or not.

Since this PR is touch the logic related to decimal points, it's suggested to make sure every related UT&UI test case is passed successfully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants