fix(tests): ensure that the latest version of the secret is used

[tonyandrewmeyer]

Issue

As of Juju 3.1.7 (and 3.3.1, and all versions from 3.4.0 onwards) the behaviour of accessing secret content in Juju has changed:

• In Juju <= 3.1.6 the secret owner would always refresh when getting the secret content, and they cannot explicitly do a refresh
• In Juju >=3.1.7 the secret owner has the same behaviour as all other secret users: they get the tracked revision unless they use peek or refresh (and they can use refresh now)

This change is already present in ops, to prepare charms for the upcoming Juju change. This change means that one of the tests (test_on_leader_elected_secrets) fails in recent versions of ops, because it gets the original secret content, and not the updated content.

Solution

The secret owner is able to use peek (in old and new ops/Juju) so the safe change is to always use peek.

[paulomach]

lgtm

[juditnovak]

Hmmm, I have a vaguely related observation... @pytest.mark.usefixtures("without_juju_secrets") decorator set (indicating that we run Juju2 tests), why this test doesn't have the @pytest.mark.usefixtures("with_juju_secrets")...? This one shouldn't pass on Juju 2 for sure :-)

BTW if we use self.charm.get_secret(), then

1. We get the content peeked
2. Instead of our assumptions on the charm's secret we get the secret the charm is supposed "think" being its secret
3. We make the test Juju2 backwards compatible

BTW @paulomach I notice that there aren't many unit tests for charm.get_secret(), charm.set_secret() at all...
There's a good set of unittests for secrets in postgres (non-charm-specific), we could copy that over to mysql charms.

[carlcsaposs-canonical]

why this test doesn't have the @pytest.mark.usefixtures("with_juju_secrets")

it's implicitly set

mysql-k8s-operator/tests/unit/conftest.py

Lines 7 to 9 in a8c9a73

	@pytest.fixture(autouse=True)
	def with_juju_secrets(monkeypatch):
	monkeypatch.setattr("ops.JujuVersion.has_secrets", True)

(we don't use a gh matrix on unit tests. more details: canonical/mysql-operator#354)

[juditnovak]

So, in this case shouldn't we clean up the outdated decorators (like on the test above) the one in question here?

[carlcsaposs-canonical]

which decorator is outdated?

[juditnovak]

Hmmm... Out of curiousity: why not to run Juju2 compatibility on unittests?

(Also, my other quetsion still holds: I think there is non-negligable value in testing the set_secret(), get_secret() iinternal nterface methods in unittests. Particularly useful when any changes on corresponding code, and this is the only place where we can execute internal interface functions direcly. Don't we wanna add (i.e. copy-paste :-) ) corresponding testing?)

[juditnovak]

Outdated decorator: @pytest.mark.usefixtures("without_juju_secrets") applied right above the test in question

[carlcsaposs-canonical]

Outdated decorator: @pytest.mark.usefixtures("without_juju_secrets")

that one's not autouse

[juditnovak]

All right. I just checked indeed the fixture changed from my orig implemenation (true that was also called only_with[out]...)

So you mean that Juju2 testing is done in a few particular cases. It's performed in a way that we override the autouse fixture (simulating Juju3) on ceratin tests -- thus flipping the environment back to Juju2.

Still my question holds regarding unittests for internal interface functions get_secret(), set_secret()... :-)

[paulomach]

@juditnovak the bulk of the tests for {get,set}_secret are in the VM operator (this file)
This is due the fact that these methods are shared between vm and k8s, but the code is owned by the vm charm

[juditnovak]

Thx much @paulomach , that was exactly what I was missing, thank U.

[dimaqq]

@tonyandrewmeyer there don't seem to be any logical changes in the current PR state.
Could it be that a fix was already included upstream?

[dimaqq]

Seems to be the case, see https://github.com/dimaqq/operator/actions/runs/9099183244/job/25011147800

[tonyandrewmeyer]

@tonyandrewmeyer there don't seem to be any logical changes in the current PR state. Could it be that a fix was already included upstream?

Possibly, yes. Feel free to check if main (here) passes with ops main (or even 2.13, we haven't done anything relevant since then). Or I can do that (but not today - I probably have time Thurs/Fri, but if not can definitely get back to this once the sprint is over).

[dimaqq]

yep, passes. let's close this.

[tonyandrewmeyer]

Also verified locally.