This is a list of all comments for UI-DEN-5638-1. Review Summary: No summary General Comment by plucia on 24 November 2020, 14:14 https://devapps.diality.us/cru/UI-DEN-5638-1#c6306 Related dialin code review: http://dvm-linux02:8060/cru/DIALIN-DEN-5638-1 General Comment by plucia on 30 December 2020, 09:30 https://devapps.diality.us/cru/UI-DEN-5638-1#c6933 As of 12/30/2020, with these changes: Code Coverage: 100% Squish Tests Passing: 100% General Comment by Behrouz NematiPour on 24 January 2021, 11:24 https://devapps.diality.us/cru/UI-DEN-5638-1#c7565 It is wrong to create an instance of HDSimulator each time you want to use any function in that class. 1 - this class has not been defined as a static class, therefore, all the listers in class will be listening for each instance and the question is which one is going to respond, or all will respond, and then we will have multiple responses. 2 -This class not only should have the "cmd_" functions as static 3 - But also it needs to be a singleton class if want to have listers. The design and usage of the class are not correct. Reply by plucia on 25 January 2021, 10:14 > Limiting the HDSimulator to static functions prevents you > from maintaining any state between successive calls and is > incompatible with the DenaliCanMessenger. It is an > improvement to hold state information and allows us to use > the DenaliCanMessenger. Using the DenaliCanMessenger and > dialin eliminates the need to maintain duplicated codebases > and allows for bidirectional messaging. Your squish folder > was doing the same thing as dialin but on a smaller scale. > > 1 - The test scripts are run in a consecutive manner, not in > parallel. Each test should be self contained. > 2 - (See statement above). This comment is about dialin, not > testsuites. > 3 - (See statement above). This comment is about dialin, not > testsuites. Reply by Behrouz NematiPour on 26 January 2021, 10:58 > I don't mean to limit the HDSimulator to static functions, > it can have static and normal methods. > {quote} > 1 - The test scripts are run in a consecutive manner, not > in parallel. Each test should be self contained. > {quote} > I agree in this case but the general implementation and > design of HDSimulator lead to that misuse of the class. Reply by plucia on 26 January 2021, 11:11 > We are in agreement that each test should be self > contained. > To ensure each test is self-contained, each test needs to > have a separate instance of HDSimulator. > The design of dialin is not the focus of this review... Reply by Behrouz NematiPour on 27 January 2021, 10:10 > RESOLVED ---------------------------------------- File: requirements.txt Revision Comment by Behrouz NematiPour on 24 January 2021, 12:06 https://devapps.diality.us/cru/UI-DEN-5638-1#c7569 what is this commit and why it has been mentioned here? how is this going to be maintained? Reply by plucia on 25 January 2021, 09:21 > At the time, this must have been the latest commit on the > dialin master branch. > It isn't maintained manually. It's generated automatically > when issuing the pip freeze > requirements.txt command. Reply by Behrouz NematiPour on 26 January 2021, 10:49 > RESOLVED ---------------------------------------- File: tst_HDBloodFlowData/test.py Revision Comment by Behrouz NematiPour on 24 January 2021, 11:38 https://devapps.diality.us/cru/UI-DEN-5638-1#c7566 What is utils.dict_update? Please use the SquishQt recommended syntax and functions as has been explained and described in their examples. I don't see any reason why we need to have a new function. Also, I would rather let SquishQt test and check the existence of the properties (in this case ".text") instead of another function adding a property to the dictionary without making sure that the object actually has the property and not checking the object tree. ** *Please remove all the usage in all other places* ** ** Reply by plucia on 25 January 2021, 10:35 > {quote} > Please use the SquishQt recommended syntax and functions as > has been explained and described in their examples. > {quote} > In this change I am actually correcting a misuse of the > squishqt api. waitForObjectExists will always return true > after the first call without using utils.dict_update. This is > because the text is omitted from the object that is being > waited for. So, the object holding the text will always > exist, and each successive call will not wait for the text to > be updated. To pass, you were basically relying on the long > delay added by cansend that occurs between each call to > test_values. That delay happened to be longer than it takes > for the text to be updated in the UI, allowing the tests to > pass by accident. This kind of test design is fragile, as the > test could fail at any time depending on how long cansend > takes to send a message. > > {quote} > I would rather let SquishQt test and check the existence of > the properties (in this case ".text") instead of another > function adding a property to the dictionary without making > sure that the object actually has the property and not > checking the object tree. > {quote} > (See above) Reply by Behrouz NematiPour on 26 January 2021, 13:00 > If there is a bug in SquishQt please report to Froglogic > and give feedback and please don't try to fix it by > yourself. > In case they fix the bug (if there is a bug) and they fix > it, and the fix is not compatible with your fix, then we > have self-generated bugs. > Please remove these and let Froglogic know you found a bug > and let them help us. Reply by plucia on 26 January 2021, 13:16 > I think you've misunderstood. The bug is not part of the > SquishQt API. It is a problem with the way the SquishQt > API is being used. Reply by Behrouz NematiPour on 26 January 2021, 13:35 > The way it was used is in SquishQt examples, but I > can't find any example that SquishQt is being used the > way you used it. > Please change it to the recommended ways in SquishQt > examples. Reply by plucia on 26 January 2021, 13:49 > Nothing has changed about the usage of the SquishQt > API, other than the fact that the dictionary passed > to waitForObjectExists previously wasn't really > testing anything. > Removing utils.dict_update it will break the test > case. Reply by Behrouz NematiPour on 26 January 2021, 14:06 > I believe we discussed enough. > And it was discussed verbally before to not to do it. > {quote} > Nothing has changed about the usage of the SquishQt > API. We pass a dictionary to waitForObjectExists > prior to this change and after. > Removing it will break the test case. > {quote} > If nothing has changed so why your test would break. > Please change it to the recommended ways in SquishQt > examples. Reply by plucia on 26 January 2021, 14:54 > I will discuss with Peman and get back to you. Reply by Behrouz NematiPour on 15 February 2021, 10:22 > Still don't feel comfortable with this solution and > haven't been convinced that it should be the way to > get around the SquishQt issue of finding the object > within the given timeout. > On the other hand, don't have a better idea to stop > the test from failing randomly. > So would be better to keep it for now. ---------------------------------------- File: tst_TreatmentStatesData/test.py Revision Comment by Behrouz NematiPour on 24 January 2021, 11:58 https://devapps.diality.us/cru/UI-DEN-5638-1#c7568 If the name needs to change, regarding our *coding standard* and also to *medical industry standards*, it should be: "TxStates" with 'x', not capital. TX has other meanings in other areas. Reply by plucia on 25 January 2021, 09:38 > I changed it from tx to TX to make it clearer it was a > constant, but to maximize readability I think it should be > TREATMENT_STATES, or TX_STATES to align with our coding > standard, not TxStates. > > From our coding standard: > {quote} > Constants are usually defined on a module level and written > in all capital letters with > underscores separating words. Examples include MAX_OVERFLOW > and TOTAL > {quote} Reply by plucia on 25 January 2021, 10:53 > Also since the variable name stylings are not considered > major issues, I will take a note and make sure to update > the variable name at a later time. Reply by Behrouz NematiPour on 26 January 2021, 10:50 > It has been defined as a class not a variable after the > name change. > So the name should be "Tx_States" Reply by plucia on 26 January 2021, 11:21 > Tx_States violates the CapWords convention for classes > as per our coding standard, as no underscores should be > present between words in a class name. > TreatmentStates is clearer than TxStates. Tx could > easily be confused with transmit... Reply by Behrouz NematiPour on 27 January 2021, 10:05 > Please change to TreatmentStates > RESOLVED ---------------------------------------- File: tst_ServiceShutdown/test.py Revision Comment by Behrouz NematiPour on 24 January 2021, 11:51 https://devapps.diality.us/cru/UI-DEN-5638-1#c7567 How come this module doesn't have "HDSimulator = HDSimulator()" and still is using an instance of the HDSimulator? it shows that : 1 - The name of the object has to be different than the class name itself so we will understand which one is used. (class or instance) 2 - If this one doesn't require defining the HDSimulator, then why it has been defined in the other ones. Reply by plucia on 25 January 2021, 09:56 > 1 - Since the name of the variable is not a major issue, I > have noted your comment and will update the name to > hd_simulator in a future revision. > 2 - It does, it was added in a future revision. Please keep > in mind this code review is 8 weeks old. Reply by Behrouz NematiPour on 26 January 2021, 10:52 > Please consider changing the instance name to hd_simulator. > The name of variables is always important, especially in > this case which makes it confusing since the name of the > class and the variable are the same. Reply by plucia on 26 January 2021, 11:08 > I agree. To ensure I don't miss additional instances of > HDSimulator I'll make this change on the latest > testsuites branch I'm working on. Reply by Behrouz NematiPour on 26 January 2021, 12:59 > RESOLVED --- ID: UI-DEN-5638-1 https://devapps.diality.us/cru/UI-DEN-5638-1 Title: UI-DEN-5638_Squish Migration Statement of Objectives: State: Closed Summary: Author: plucia Reviewers: (0 active, 2 completed*) Behrouz NematiPour (*) pmontazemi (*)