Code review comment for lp:~rvb/maas/agent-bug-1239488-acquire

Revision history for this message
Gavin Panella (allenap) wrote :

> > Can we instead sort out these invariants in Node.clean()? For example,
> > Node.agent_name would always be "" if status != ALLOCATED.
>
> I think this is a very good point.  I'd rather tackle that in a follow-up
> branch though.

Sure, that's fine.

> This is of no consequence now because we don't use
> NODE_STATUS.RETIRED or NODE_STATUS.MISSING.

Then we should remove them (in another branch). They're cognitive
baggage right now.

>
> > [2]
> >
> > +    def test_POST_release_resets_agent_name(self):
> > ...
> > +    def test_POST_acquire_sets_agent_name(self):
> > ...
> > +    def test_POST_acquire_agent_name_defaults_to_empty_string(self):
> >
> > These are fine, but they're testing model code indirectly too. There
> > are no unit tests for the changes to the Node model. I think we ought
> > to nail down the behaviour of agent_name to the model code.
>
> Well, I also test the changed behavior of the model's code.  See the changes
> to test_node.py.

So you have. I'm going code-blind :-/

>
> test_POST_release_resets_agent_name is a test I /could/ remove… but I don't
> think a little bit of duplication hurts if it means thorough testing of the
> expected behavior.

Yeah, that's fine.

« Back to merge proposal