search

knime / knimepy

Other
44 stars 15 forks source link

Alter exception message to indicate workflow locked by another instance #21

Closed applio closed 4 years ago

applio commented 4 years ago

Implements enhancement suggested in issue #20 and provides test.

applio commented 4 years ago

Will add suggestion from @MarcelWiedenmann to check for "KNIME" in the exception args/text as an additional verification that it really was the ChildProcessError we expected and not one triggered from somewhere else.

applio commented 4 years ago

It appears the suggested check is already present in the unit test, immediately after trapping the ChildProcessError:

        self.assertEqual(
            cm.exception.args[0],
            knime.KEYPHRASE_LOCKED.decode('utf8')
        )
MarcelWiedenmann commented 4 years ago

Oh yeah, I meant adding it to #13. Somewhere below the block here and the one below.

applio commented 4 years ago

Ah! Sorry for mixing those up.

With #21 merged, we have a positive test to verify that a locked workflow conflict raises the appropriate exception and message.

In #13, if anything we would want to verify that the ChildProcessError is not the result of a locked workflow conflict otherwise the test might accidentally pass. I will add this to the notes on #13 as well but I did not see a message in the logs / stderr that I felt we could trust to be there but I wish there were a way to create such a positive test rather than a negative one.