pcdshub / IocManager

pyqt5 + pyca-based EPICS IOC Manager
https://confluence.slac.stanford.edu/display/PCDS/IOC+Manager+for+Users
2 stars 6 forks source link

fix(imgr): tragically switched the argument order on a function call #10

Closed joshc-slac closed 1 year ago

joshc-slac commented 1 year ago

Description

Check commit message. Unfortunate switch up of argument ordering in passing to function call.

Theoretically we could exception handle file io or we could test our code and be clever enough not to make mistakes like this.

tangkong commented 1 year ago

This repo could use a number of enhancements, and a test suite might be top of the list. What is the testing pattern for this app anyway? There doesn't seem to be a dry-run mode or anything similar, so opening up IocManager and actually making changes seems like the main test method

While exception handling is always appreciated, I think in this case the exception was descriptive and appropriately severe (halting execution). Just my 2c anyhow

joshc-slac commented 1 year ago

Largely agree. I was thinking I'd love to implement gtest in this repo but doing so in python2.7 seems prohibitively painful, that may be a misevaluation. I figure most of the senior devs here are familiar enough with our prod system to test their features without breaking things, this is somewhat of a barrier to access for other folks depending on how steep the learning curve is (still evaluating this).

Where I somewhat disagree: exception handling wrapping IO would be very nice just based on how many different machines we deploy on with their own permutable environments. If we had a shared python utility library it would be easy to share across repos. However because we are in the paradigm of "engineers just keep things working / logically coherent" its probably low value add atm.

mcb64 commented 1 year ago

This repo could use a number of enhancements, and a test suite might be top of the list. What is the testing pattern for this app anyway? There doesn't seem to be a dry-run mode or anything similar, so opening up IocManager and actually making changes seems like the main test method

Unfortunately, yes. There is no reason why we couldn't do actual testing with a fake hutch and fake IOCs though. Just a lack of time and will stop us. (An "IOC" for this purpose could be any bash/python script that doesn't exit and somehow identifies itself by writing a status file. A test suite could easily be developed to add several of these scripts, upgrade them, stop them, etc.)

If we don't improve our testing regime, we will be bitten by problems like this time and time again. Typos are hard to find by just reading code.

tangkong commented 1 year ago

Where I somewhat disagree: exception handling wrapping IO would be very nice just based on how many different machines we deploy on with their own permutable environments. If we had a shared python utility library it would be easy to share across repos. However because we are in the paradigm of "engineers just keep things working / logically coherent" its probably low value add atm.

I guess my thoughts were focused specifically on the error we saw here: a file-not found error. In this case even if we caught this particular exception, we'd want to abort the program anyhow.

I'm not suggesting we live in the "hotfix everything on the fly" world, don't misunderstand me 🙏

mcb64 commented 1 year ago

I'm not suggesting we live in the "hotfix everything on the fly" world, don't misunderstand me 🙏

Well... we shouldn't live in this world... I'm not convinced that we don't.

joshc-slac commented 1 year ago

This repo could use a number of enhancements, and a test suite might be top of the list. What is the testing pattern for this app anyway? There doesn't seem to be a dry-run mode or anything similar, so opening up IocManager and actually making changes seems like the main test method

Unfortunately, yes. There is no reason why we couldn't do actual testing with a fake hutch and fake IOCs though. Just a lack of time and will stop us. (An "IOC" for this purpose could be any bash/python script that doesn't exit and somehow identifies itself by writing a status file. A test suite could easily be developed to add several of these scripts, upgrade them, stop them, etc.)

If we don't improve our testing regime, we will be bitten by problems like this time and time again. Typos are hard to find by just reading code.

I hear someone is working in the free time on a docker image to do this testing localy however even a testing server like the one I know we have setup would be sufficient. The real issue was I rushed and this was my bad we do have ways to test it and the more we test the easier itll be. Good lesson for me to take moving forward!