-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
base: main
Are you sure you want to change the base?
Conversation
Since we have Add @xuhongxu96 to help review on this :) |
I corrected my code to use |
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 BTW, since your 1st commit succeeded building, I doubt the failure was caused by some newer packages you have updated in the later commits. |
@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? |
@DeclanGazil02 No problem at all and let's make it merged!
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. |
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. Thanks again to your interests and your contribution and looking forward to seeing this PR gets completed. |
@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 |
Have you installed the WinAppDriver? It is a fundamental tool that UI test cases rely on. And other points to pay attention to are:
|
I downloaded WinAppDriver @tian-lt, and now the only tests that are failing are due to |
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. |
@tian-lt now all tests are actually running. 10 tests are failing but due to wrong output. For example |
I also updated my test settings as you mentioned. Thank you for the in-depth walk through! |
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. |
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:
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: