[executor-preview] Test noise_learner_v3.py

[yaelbh]

Summary

Part of #2479.
Tests for the functions in the file qiskit_ibm_runtime/noise_learner_v3/noise_learner_v3.py.

Details and comments

[SamFerracin]

Consider making these properties public. Do the other primitives do that?

[yaelbh]

They are private for the other primitives.

[SamFerracin]

I can't think of a reason for them to be private, so I would expose them as properties (obviously without setters). Accessing private attributes is something that we should avoid in every case

[yaelbh]

I'll do it. Anyway in a test, when we want to verify that the internals of a class were properly initialized, I think it does make sense to query private variables. I have the same story also in #2501.

[yaelbh]

There was already a backend function, not a property. It's better as a property but I don't want to cause a breaking change. In 4de1380 I wrote session and service as properties, maybe change them to functions too, just for uniformity with backend?

[SamFerracin]

Was the part above redundant, because_get_mode_service_backend just undoes what was done above?

[yaelbh]

The part above was some copying of the content of _get_mode_service_backend, with some editing, that introduced bugs. Some cases were addressed (correctly or incorrectly) by the part above, without ever reaching the call to _get_mode_service_backend. Other cases did reach the call, and went through the same process again inside the function.

[SamFerracin]

I'm not sure what happens if self._backend is a simulator, I would expect it to raise somewhere (init or run? 🤷). The truth is that we never supported simulators for NoiseLearner, and the same applies to NoiseLearnerV3.

If this is indeed the case, I would prefer to add a check to the init along the lines of:

if is_simulator(self._backend):
    raise ValueError("Simulator backend is not supported.")

And then here:

• We would not need to check is_simulator anymore
• We would need to raise ValueError if target or configuration is None, because real backends do have a target and a config

[yaelbh]

• We already raise, in init, if we fall on the QiskitRuntimeLocalService, perhaps it covers the case of a simulator?
• If real backends always have a target and a config then why check it?

[yaelbh]

I don't think it can be a simulator at this point, so I removed the check in 4b98826. I also think the check if target and config are None is redundant, but why not. We can also revert and think about all this in a separate issue.

[SamFerracin]

I can't think of a reason for them to be private, so I would expose them as properties (obviously without setters). Accessing private attributes is something that we should avoid in every case

[SamFerracin]

I wonder why do we need to reset (twice) something that we just initialized. We should check (in another issue) if we can improve this mock

[yaelbh]

I copied it from the primitives v2 tests. We should check what happens in this regard when tests run in parallel.

[yaelbh]

I'll check

[yaelbh]

Indeed I don't see a reason for any of the calls to reset_mock in test_noise_learner_v3.py, and for most of the calls in test_ibm_primitives_v2.py. To be on the safe side, as you say, we'll face it in another issue.

[SamFerracin]

In this test and in the next one you are manually monkey-patching by overwriting service._run. This is not good practice as it can have undesirable side effects, such as leakage (the manual change persists beyond the single test case that made it).

Please use unittest.mock, which takes care of the cleanup. There are some examples in other files

[yaelbh]

I don't see how a leakage can occur. The mocked session is local only to this test. But I agree that the good practice is do something like done in c47364c. By the way note that we override session data members inside get_mocked_session.