civisanalytics / civis-python

Civis API Python Client
BSD 3-Clause "New" or "Revised" License
34 stars 26 forks source link

FIX get_table_id should correctly handle quoted schema.tablename #285

Closed jacksonllee closed 5 years ago

jacksonllee commented 5 years ago

Resolves #276

jacksonllee commented 5 years ago

All Travis builds have passed. Waiting for AppVeyor builds to finish: https://ci.appveyor.com/project/civisanalytics/civis-python/builds/22468450

jacksonllee commented 5 years ago

The AppVeyor builds have finished:

screen shot 2019-02-19 at 11 18 34 am

The Python 3.6 build failed, but the issue was the sort of non-deterministic test behavior we've seen from time to time that I think we can safely ignore for this PR:

================================== FAILURES ===================================
_____________________________ test_custom_scripts _____________________________
    def test_custom_scripts():
        with mock.patch.dict('os.environ', {'CIVIS_JOB_ID': '12',
                                            'CIVIS_RUN_ID': '40'}):
>           c = _check_executor(133)
civis\tests\test_futures.py:292: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
from_template_id = 133
    def _check_executor(from_template_id=None):
        job_id, run_id = 42, 43
        c = _setup_client_mock(job_id, run_id, n_failures=0)
        mock_run = c.scripts.post_containers_runs()
        if from_template_id:
            bpe = CustomScriptExecutor(from_template_id=from_template_id,
                                       client=c, polling_interval=0.01)
            future = bpe.submit(my_param='spam')
        else:
            bpe = _ContainerShellExecutor(client=c, polling_interval=0.01)
            future = bpe.submit("foo")

        # Mock and test running, future.job_id, and done()
        mock_run.state = "running"
>       assert future.running(), "future is incorrectly marked as not running"
E       AssertionError: future is incorrectly marked as not running
E       assert False
E        +  where False = <bound method Future.running of <ContainerFuture at 0x23e50ed518 state=succeeded returned Response>>()
E        +    where <bound method Future.running of <ContainerFuture at 0x23e50ed518 state=succeeded returned Response>> = <ContainerFuture at 0x23e50ed518 state=succeeded returned Response>.running
civis\tests\test_futures.py:257: AssertionError

This PR is ready for review.

stephen-hoover commented 5 years ago

It looks like there's still something going on with Travis. Why does it need to run tests on both your branch and the PR? Aren't those the same code? Maybe I'm misunderstanding what "Branch" and "Pull Request" mean there.

jacksonllee commented 5 years ago

I've contacted IT-Support to help sort out this repo's config (hopefully once and for all) after I'd noticed merging is still blocked. Will report back as soon as I have updates.

jacksonllee commented 5 years ago

According to IT-Support, apparently there are indeed both "branch" and "pull request" options for Travis CI checks, and somehow "branch" was checked -- it is not anymore, and hence everything green now.