An important difference with this path is that we must close file_obj if we open it based on a path, but we must not close file_obj if we are given a file-like object.

I recommend using a with statement when we are provided a path to make sure we always close it. You may want to refactor this to add a _schema_from_json_file_object() private helper method that does the loading part without the file closing part.

Added helper method per recommendation along with the statement to close the file.

In general we use ValueError for unexpected input argument.

Updated with correct exception.

I'd prefer if we let this raise (don't catch it). That way the user doesn't lose context about what is wrong with the input file.

Took this piece out.

Just as in schema_from_json, we must close file_obj if we open it based on a path, but we must not close file_obj if we are given a file-like object.

Updated with with statement to close when appropriate.

You're basically repeating the function definition here. That's not ideal. You've basically written a change-detector test. I'd prefer to see actual schema.SchemaField constructors in tests.

I do like that you've mocked out open and the file contents, so keep that. Just change how you construct expected.

Made changes to include schema.SchemaField as requested.

Nit: since the function name is schema_from_json, the test name should be test_schema_from_json (just one underscore between test and schema.)

Ditto for schema_to_json test function name.

Updated test names appropriately.

Nit: This and the other docstrings (except for the first line) look a like they are indented 4 spaces too many.

Fixed indentations.

Nit: since file is a built-in, use file_ or file_obj as the name.

Changed name as directed.

FYI: This loop could be replaced with a Python "list comprehension".

Changed to be a list comprehension.

Nit: file should be file_obj since file is a built-in.

Updated name accordingly.

Nit: Since the above line returns, no need for else.

Removed the else as recommended.

Use json.dump(schema_list, destination) instead. Then you can use BytesIO in the tests, too.

I'd prefer if json.dump wasn't repeated (here we do want to be DRY). Replace this with file_obj = destination and remove the else.

Refactored into a helper function because I couldn't get it to work with the context manager below just by removing the else.

No need for .append. Instead, construct the list inline.

expected = [
    SchemaField(...),
    SchemaField(...),
    ...
]

Switched to suggested method.

We don't care about the actual arguments, so just assert_called_once will work.

Removed the arguments from the assert.

Might need wb when using json.dump. (b for "binary" mode)

Wonderful!

One nit-pick, though. Style-wise in our client libraries and samples, we avoid single-letter variable names, even in list comprehensions. Let's rename f to field.

Variable name has been updated!

I like your thinking here. It's definitely nicer to compare lists than it is to compare JSON strings.

Nit: Let's write both with statement on one line. https://stackoverflow.com/a/1073814/101923

Nit: No need for leading underscore, that's usually to indicate a "private" variable, which isn't relevant inside a test.

with open_patch as mock_file, mock.patch("json.dump") as mock_dump:

Replace existing with statements with suggested line.

Let's call json.loads(fake_file.getvalue()) and compare to a list of dictionaries like you do in the test of the path version.

The test has been updated with the list of dictionaries and updated assert.

Oops, coverage is failing on this line and the similar line in the other function. We'd need a test where open() fails.

Honestly, I'd be okay removing the try block and letting these errors just raise, too.

Removed the try block as suggested for both functions.

From the test logs, it looks like this is tripping up Python 2. https://stackoverflow.com/a/34677735/101923

=================================== FAILURES ===================================
____________ TestClientUpload.test_schema_from_json_with_file_path _____________
self = <tests.unit.test_client.TestClientUpload object at 0x7f47e2dce4d0>
    def test_schema_from_json_with_file_path(self):
        from google.cloud.bigquery.schema import SchemaField
        file_content = """[
          {
            "description": "quarter",
            "mode": "REQUIRED",
            "name": "qtr",
            "type": "STRING"
          },
          {
            "description": "sales representative",
            "mode": "NULLABLE",
            "name": "rep",
            "type": "STRING"
          },
          {
            "description": "total sales",
            "mode": "NULLABLE",
            "name": "sales",
            "type": "FLOAT"
          }
        ]"""
        expected = [
            SchemaField("qtr", "STRING", "REQUIRED", "quarter"),
            SchemaField("rep", "STRING", "NULLABLE", "sales representative"),
            SchemaField("sales", "FLOAT", "NULLABLE", "total sales"),
        ]
        client = self._make_client()
        mock_file_path = "/mocked/file.json"
        open_patch = mock.patch(
            "builtins.open", new=mock.mock_open(read_data=file_content)
        )
>       with open_patch as _mock_file:
tests/unit/test_client.py:5202:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
.nox/unit-2-7/lib/python2.7/site-packages/mock/mock.py:1353: in __enter__
    self.target = self.getter()
.nox/unit-2-7/lib/python2.7/site-packages/mock/mock.py:1523: in <lambda>
    getter = lambda: _importer(target)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
target = 'builtins'
    def _importer(target):
        components = target.split('.')
        import_path = components.pop(0)
>       thing = __import__(import_path)
E       ImportError: No module named builtins

Remember you can run against Python 2 by using nox, locally.

We could check the Python version with sys.version_info.major and change what object you are patching.

Actually six.PY2 is probably a better way to detect. https://pythonhosted.org/six/

Fixed all the issues and the code is compatible with python 2 now.