Convert LocalJob tests into unit-tests. #526
Conversation
| import concurrent.futures as futures | ||
| import qiskit.backends.local.localjob as localjob | ||
|
|
||
| executor = unittest.mock.MagicMock(spec=futures.Executor) |
There was a problem hiding this comment.
This seems like the right way to do this.
| def mocked_simulator_binaries(): | ||
| """Context to force binary-based simulators to think the simulators exist. | ||
| """ | ||
| from qiskit.backends.local import qasm_simulator_projectq |
There was a problem hiding this comment.
Was the choice of the binary simulator here arbitrary? If so, it may be better to use a non-third-party one.
There was a problem hiding this comment.
Right although, technically, I'm not testing the simulators but all the backends that come with QISKit regardless of which simulator they are interfacing with. That's why I'm patching the objects pretending the simulators are there. Does it make sense?
There was a problem hiding this comment.
I think I see what this is doing now.
There was a problem hiding this comment.
I think the implementation is neat, although the intent might fall a bit outside the principles applied to the rest of the suite - in general we rely on the fact that if a backend cannot be instantiated, it means is "optional" or not available on the current environment: however, during the second stage of Travis we do ensure the cpp sims are available anyway.
So in practice the mocking would only affect the projectq simulator most of the time (which is bound to be removed from the core as well), and might be a bit tricky to keep the mocking robust enough depending on how the codebase evolves - it is fine for me to leave it as it is, but might make sense to just simplify and avoid the mocking in the future 👍
There was a problem hiding this comment.
So, do you prefer the tests for these cases to be optional?
1ddf9ea to
1bb00ae
Compare
|
@diego-plan9 or @atilag can you review my changes in the |
03f449c to
c71b67e
Compare
|
Ok, I made the code compatible with Python 3.5. |
test/python/test_localjob.py
Outdated
|
|
||
|
|
||
| @contextmanager | ||
| def intercepted_executor_for_LocalJob(): |
There was a problem hiding this comment.
Can you rename this method to snake case (intercepted_executor_for_localjob)? There are a number of other variable names that have also been hidden by the disable=invalid-name directive at the top (backendConstructor, mockedFuture, mockedCallable) - in the tests we are not too strict about the linter (as, for example, the assertCalledOnce is a valid name despite nor being technically in line with the linter preferences), but in general we should adhere to those.
There was a problem hiding this comment.
The case for assertCalledOnce is that it adheres to the API convention for assertion ids in the Test class but I can change it if needed.
There was a problem hiding this comment.
Sure - I was actually trying to say that assertCalledOnce is an example of a reasonable exception to the linter rule and makes sense to keep it in camel-case :)
test/python/test_localjob.py
Outdated
| patch.object(LocalJob, '__init__', return_value=None, | ||
| autospec=True): | ||
|
|
||
| for backendConstructor in self._backends: |
There was a problem hiding this comment.
Can you use with self.subTest() here? Mostly for allowing better handling of error in case the test fails, as it allows each assertion inside the block to be tested independently.
test/python/test_localjob.py
Outdated
| quantum_job = QuantumJob(qobj, backend, preformatted=True) | ||
| job = backend.run(quantum_job) | ||
| self.assertTrue(job.backend_name == backend_name) | ||
| # Like before. |
There was a problem hiding this comment.
Nit: can you expand this line and make it a bit more informative?
It uses the unittest.mock module to patch external dependencies. It also uses auto-speccing [1] to ensure the mocks expose the same API as the objects being mocked. [1] https://docs.python.org/3/library/unittest.mock.html#autospeccing
c71b67e to
78738da
Compare
There was a problem hiding this comment.
Thanks, @delapuente ! I have adjusted a variable that seemed to have escaped your latest commit, and also removed the sympy simulators due to the recent merging of #514 so ... we are all set! 🎉
* Convert LocalJob tests into unit-tests. It uses the unittest.mock module to patch external dependencies. It also uses auto-speccing [1] to ensure the mocks expose the same API as the objects being mocked. [1] https://docs.python.org/3/library/unittest.mock.html#autospeccing * Make code compatible with Python 3.5 mock API * Adding subtest, fixing names and clarifying some tests. * Fixing variable names * Fix lint, remove sympy simulators
It uses the unittest.mock module to patch external dependencies. It also uses speccing to ensure the mocks expose the same API as the objects bring mocked.
Motivation and Context
Current tests behave as integration tests so, in case of failure, it is not guaranteed the problem arises from LocalJob-related abstractions malfunctioning.
More important, current test suite indirectly tested Python standard library
ExecutorandFutureclasses which are already tested by the Python test suite. We need to test if our encapsulation is using the Futures API as intended for concurrent execution.Checklist: