DM-17932: Validate object type when reading objects from pickle #16
Conversation
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def makeTmpFile(): |
There was a problem hiding this comment.
Is there a reason you chose to make your own Context manager instead of using tempfile.TemporaryFile ?
There was a problem hiding this comment.
The reason is that I need the name of the file and not file object. It could potentially be done with NamedTemporaryFile but it still needs additional code to close the file, so I prefer old-fashioned mkstemp instead in a separate context manager.
tests/test_cmdLineFwk.py
Outdated
| pickle.dump({}, pickleFile) | ||
| args = _makeArgs(pipeline=tmpname) | ||
| with self.assertRaises(TypeError): | ||
| pipeline = fwk.makePipeline(taskFactory, args) |
There was a problem hiding this comment.
Small nitpick, but a statement assigning this to a variable seems needless for this and the test below.
|
|
||
| # make empty graph and store it in a file | ||
| qgraph = QuantumGraph() | ||
| with open(tmpname, "wb") as pickleFile: |
There was a problem hiding this comment.
Would this be better to do like the test above where you using the save the graph by using the save_graph argument to _makeArgs?
There was a problem hiding this comment.
The CmdLineFwk.makeGraph() method cannot make empty QGraph, and to make non-empty one it needs butler which we do not have yet implemented for unit tests. So the only way to save QGraph to a pickle file now is to make an instance manually and write it ourselves. That is simple enough for this unit test.
Trivial change to the code. Also adds less trivial unit test for these checks to validate validation. CmdLineFwk unit tests will be expanded later with more tests for other methods (some will need reasonable registry contents).
andy-slac
force-pushed
the
tickets/DM-17932
branch
from
19ce6b0
to
8f63b41
Compare
Trivial change to the code. Also adds less trivial unit test for these
checks to validate validation. CmdLineFwk unit tests will be expanded
later with more tests for other methods (some will need reasonable
registry contents).