Fix unit tests after recent ops update #22
Conversation
As of canonical/operator#1313 , the test harness inserts JUJU_VERSION=0.0.0 into os.environ, so we must account for this in the unit tests.
samuelallan72
requested review from
Pjack,
aieri,
jneo8,
gabrielcocenza,
rgildein and
chanchiwai-ray
There was a problem hiding this comment.
The changes itself are ok, but I'm wondering if instead of fixing into our projects we should open a bug on ops framework. I say that because there are lots of "JUJU_" env vars and why we need to always set only the JUJU_VERSION? Shouldn't be optional? What is the advantage of having JUJU_VERSION as 0.0.0?
IMO the change on Ops wasn't suppose to break this kind of test
We don't; we're only adding it to the assert statement here. Ops test framework adds it by default I guess to not break things in ops since they changed the way they use manage context internally? It's an internal thing anyway as far as I can tell; I don't think anything has changed functionally. Although you're right - I don't know why the need to manipulate the environment in the test framework. 🤔 |
|
FYI, we're going to do an ops 2.16.1 release (hopefully today, but definitely by Monday) that reverts the Harness change, so you might want to hold off on this, or otherwise you should be able to revert it after that. Thanks for bringing this to our attention! |
Harness needs to provide a `JUJU_VERSION` value to set up the `_JujuContext`, but it doesn't have to be in `os.environ`, it can just be in the dictionary passed to create the `_JujuContext` object. In production, `os.environ` would actually have this, so it's more realistic for it to be present, but it would also have lots of the other `JUJU_` environment variables as well, and we don't want to have Harness simulate all those in the environment - we want people working with the ops tools to access those, not the environment directly. This change [broke the tests of at least one charm](canonical/charm-simple-streams#22) because it patches the environment to have specific values, and then creating the `Harness` object changes that. It seems better for us to not do this - if we did want to populate the environment to mimic Juju then we'd likely want that to be explicit, or done around the event emitting. It was also an accidental backwards compatiblity break.
Harness needs to provide a `JUJU_VERSION` value to set up the `_JujuContext`, but it doesn't have to be in `os.environ`, it can just be in the dictionary passed to create the `_JujuContext` object. In production, `os.environ` would actually have this, so it's more realistic for it to be present, but it would also have lots of the other `JUJU_` environment variables as well, and we don't want to have Harness simulate all those in the environment - we want people working with the ops tools to access those, not the environment directly. This change [broke the tests of at least one charm](canonical/charm-simple-streams#22) because it patches the environment to have specific values, and then creating the `Harness` object changes that. It seems better for us to not do this - if we did want to populate the environment to mimic Juju then we'd likely want that to be explicit, or done around the event emitting. It was also an accidental backwards compatiblity break.
ops 2.16.1 is out now and should remove the need for this change. |
|
@tonyandrewmeyer thanks for the quick fix in the Ops framework. Appreciate your help! |
As of canonical/operator#1313 , the test harness inserts JUJU_VERSION=0.0.0 into os.environ, so we must account for this in the unit tests.