This is a great change! I have a bunch of comments, but they're mostly just style nits.

Should explicitly say "review request" instead of just "request"

The loaded config should already exist as self.config

Add a blank line between these two.

When we do multi-line conditionals, we prefer parens rather than the continuation character:

if (self.options.target_groups is None and
    'TARGET_GROUPS' in config):
    ....

Add a blank line between these two.

Parens here too.

This can just be '.reviewboardrc' instead of './.reviewboardrc'
That said, do we need to make sure that this is running within a temporary directory? What is the cwd when these tests run?

In order to avoid leaving the temporary file around in case the test case fails unexpectedly, this should probably be:

try:
    yield
finally:
    tmp.close()
    os.unlink('.reviewboardrc')

Should include the type:
args (list):

Not all arguments have values. I'd just say that the command line will include each of the items within the list.

Each of your new test cases needs a docstring in order to print a description when running the test suite. See the above tests for examples of what they should look like.

Add a blank line between these two.

Add a blank line between these two.

Add a blank line between these two.

Concept behind the change certainly seems reasonable.
I wanted to touch upon the Notion issue. It should be public. What link are you trying? I want to figure out if something regressed.

Something went a little weird when posting your latest revision. It looks like you ended up posting the changes only in the latest commit and not the full diff between master and your topic branch (probably via something like rbt post HEAD). This breaks the interdiffs and doesn't let us use rbt land to land the change. Would you mind re-posting by running rbt post -r 9442 master..mybranch?

We don't compare booleans against True or False. Instead, this should just do if not self.options.update

Private methods should go at the end of the class. That said, I think this is better as a public function in RBTestBase, so that other commands can benefit. I'd call this just reviewboardrc.
This actually should apply to _create_post_command as well, even though it wasn't following this rule before.
This also needs a docstring.

This can be hard-coded in the open call.

The issue with a chdir is that any command being tested will no longer have access to the source code repository, if it's testing against one. I'd make the chdir optional. If the unit test is cloning a repository and doing a test in there, it's fine for it to write to that same directory.

This would be better as:

with open(test_file, 'w') as fp:
    for ...:
        ...

The file should import six, and then this would be:

for key, value in six.iteritems(data):
    ...

A couple important things to note here:

This is assuming only strings are passed as values, which isn't always going to be true. Sometimes it will be booleans or lists.
It also needs a newline, or multiple values are going to create an invalid file.

Therefore, I'd switch this to:

fp.write('%s = %r\n' % (key, value))


The %r will generally return something suitable for the kinds of values we're working with. Down the road, we'll probably need to have a better serializer, but this is fine for now.
(We generally use printf-style strings anyway.)

Is there a reason to flush above and then only close now? I'd suspect we'd want to just close the file above (which my with suggestion will do).

This isn't needed. The rmdir will be sufficient.

This will fail if anything else is written to the directory. We should use shutils.rmtree, and only do this if that optional parameter I mentioned is provided.

Should be list of unicode.

Docstrings for unit tests shouldn't end in a period.
Here and below.

Instead of just having True, can we do use_temp_dir=True?

Same here.

Same here.

Same here.

Just one last nit from me:

It was like this before, but this return type/description is incorrect. It returns the command instance.

Ship It!