Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added unit/integration tests for python3.7 #8217

Merged
merged 2 commits into from Sep 7, 2018
Merged

Added unit/integration tests for python3.7 #8217

merged 2 commits into from Sep 7, 2018

Conversation

Karel-van-de-Plassche
Copy link
Contributor

See title. Not sure if python3.7 support is premature at this point, but I found some functions failing that worked in python3.6, so this is the first step in testing compatibility.

@Karel-van-de-Plassche
Copy link
Contributor Author

And as always, immediately find my mistake after submitting a pull request. It was actually unrelated to my upgrade to python3.7 but a silly mistake on my end related to #5439. Solved! But still would be good to test for python3.7 early on I guess.

@bryevdv
Copy link
Member

bryevdv commented Sep 6, 2018

I'm not sure about the value of adding another job for integration tests. The intergration tests are really testing BokehJS and browser interaction more than anything. If there are any compat issues with actual python code, I'd expect them to show up in python unit tests (modulo any missed coverage)

@Karel-van-de-Plassche
Copy link
Contributor Author

Ah, okay. I was unsure about that. Let's see if the current Travis job succeeds, and just remove the integration test for 3.6 or 3.7 if it does.

@bryevdv
Copy link
Member

bryevdv commented Sep 6, 2018

Sure, I'd be +1 in adding it some way or other. We could

  • add just a py 3.7 unit test job (marginal increase in build time)
  • change the current 3.6 unit test job to 3.7 (and let the integration test run 3.6)

The latter would not affect build times, and still provide a measure of coverage across all versions. Between the two I guess I'd prefer the second but would be fine with the first if others prefer it.

@Karel-van-de-Plassche
Copy link
Contributor Author

I'd actually be in favor of adding the 3.7 job, as then we can be sure that any failure of integration tests is really because of the integration, not of some weird python3.7<>3.6 differences (however unlikely)

@bryevdv
Copy link
Member

bryevdv commented Sep 6, 2018

We will have to try bumping the conda-build version:

UnsatisfiableError: The following specifications were found to be in conflict:
  - conda-build=3.0.27 -> enum34 -> python[version='>=2.7,<2.8.0a0']
  - python=3.7
Use "conda info <package>" to see the dependencies for each package.

I'll make the change on Travis and restart

@Karel-van-de-Plassche
Copy link
Contributor Author

Very strange error though, python[version='>=2.7,<2.8.0a0'] implies it would also fail for python3.6, but it doesn't..

@bryevdv
Copy link
Member

bryevdv commented Sep 6, 2018

Unfortunately something about latest conda-build tiriggers this in the job:

+popd
~/build/bokeh/bokeh
+conda build conda.recipe --quiet --no-test --no-anaconda-upload
Bokeh includes a JavaScript library (BokehJS) that has its own
build process. How would you like to handle BokehJS:
1) build and install fresh BokehJS
2) install last built BokehJS from bokeh/bokehjs/build
Choice? 

@Karel-van-de-Plassche
Copy link
Contributor Author

I'll also have a look locally

@bryevdv
Copy link
Member

bryevdv commented Sep 6, 2018

All of the work is supposed to be skipped during conda build collection by this:

# if this is just conda-build skimming information, skip all this actual work
if not conda_rendering():
    fixup_for_packaged()   # --build_js and --install_js not valid FROM sdist
    fixup_building_sdist() # must build BokehJS when MAKING sdists

    bokehjs_action = build_or_install_bokehjs()

but it seems as though conda_rendering is not returning True anymore

@bryevdv
Copy link
Member

bryevdv commented Sep 6, 2018

This is a bug in conda-build, I've restored the versions of CONDA_REQS for now, so other jobs still work. We can resume this after that next conda-build with a fix is released.

@Karel-van-de-Plassche
Copy link
Contributor Author

Ah, okay. I find it a bit weird the setup.py is being called so many times during conda build, but I'm sure there is a good reason for that by the conda people ;) Just printing out conda_rendering() in setup.py and commenting out the BokehJS build stuff:

conda build conda.recipe --quiet --no-test --no-anaconda-upload
False
False
False
False
False
False
True
False
False
True
False
False

@bryevdv
Copy link
Member

bryevdv commented Sep 6, 2018


Actually looks like he already has a fix

@Karel-van-de-Plassche
Copy link
Contributor Author

Nice, very quick solution! I did a bisect of conda-build versions, and unfortunately everything newer than 3.0.31 has the same bug, and this version has the same conflict with python3.7. Let's wait until the next conda-build release indeed.

@bryevdv
Copy link
Member

bryevdv commented Sep 6, 2018

@Karel-van-de-Plassche ok, build is working with new conda-build. There is now this unusual failure on both 3.6 and 3.7, which was also observed in the windows test PR. I thought it was a windows issue but apparently not:

____________ Test_Document_delete_modules.test_extra_referrer_error ____________
self = <bokeh.document.tests.test_document.Test_Document_delete_modules object at 0x7fa45900a668>
caplog = <_pytest.logging.LogCaptureFixture object at 0x7fa45900a4a8>
    def test_extra_referrer_error(self, caplog):
        d = document.Document()
        assert not d.roots
        class FakeMod(object):
            __name__ = 'junkjunkjunk'
        mod = FakeMod()
        import sys
        assert 'junkjunkjunk' not in sys.modules
        sys.modules['junkjunkjunk'] = mod
        d._modules.append(mod)
        assert 'junkjunkjunk' in sys.modules
    
        # add an extra referrer for delete_modules to complain about
        extra.append(mod)
        import gc
>       assert len(gc.get_referrers(mod)) == 4
E       AssertionError: assert 3 == 4
E        +  where 3 = len([[<bokeh.document.tests.test_document.Test_Document_delete_modules.test_extra_referrer_error.<locals>.FakeMod object a...tests.test_document.Test_Document_delete_modules.test_extra_referrer_error.<locals>.FakeMod object at 0x7fa458f90a20>]])
E        +    where [[<bokeh.document.tests.test_document.Test_Document_delete_modules.test_extra_referrer_error.<locals>.FakeMod object a...tests.test_document.Test_Document_delete_modules.test_extra_referrer_error.<locals>.FakeMod object at 0x7fa458f90a20>]] = <built-in function get_referrers>(<bokeh.document.tests.test_document.Test_Document_delete_modules.test_extra_referrer_error.<locals>.FakeMod object at 0x7fa458f90a20>)
E        +      where <built-in function get_referrers> = <module 'gc' (built-in)>.get_referrers
bokeh/document/tests/test_document.py:120: AssertionError

No idea why it is sometimes suddenly failing. The test is to ensure there are not extra referrers to a bokeh app module. However the error is actually that there are fewer than expected referrers. A quick fix is to change ==4 to <=4... maybe that's fine? I would be worried about too many, but having less just seems like a good thing, all things being equal. (tho I would be curious as to the root cause).

@bryevdv
Copy link
Member

bryevdv commented Sep 6, 2018

That is super weird tho, there woudl seem clearly to be four referrers:

  • locals
  • extra list
  • the document
  • get_rererrers reports one for itself

@bryevdv
Copy link
Member

bryevdv commented Sep 6, 2018

OK this failure is actually specific to py3.7. The python 3.6 test is currently inadvertently installing py3.7

@bryevdv
Copy link
Member

bryevdv commented Sep 6, 2018

@Karel-van-de-Plassche can you rebase/merge master, and make the change discussed above? At that point, only the 3.7 unit test should fail

@bryevdv
Copy link
Member

bryevdv commented Sep 7, 2018

@Karel-van-de-Plassche I am handling the one unit test issue in #8222 After that is merged, you can merge/rebase master, then this PR should be ready to go as well.

@bryevdv bryevdv added this to the 1.0 milestone Sep 7, 2018
@bryevdv bryevdv merged commit 150e60f into master Sep 7, 2018
@bryevdv bryevdv deleted the karel/python3.7 branch September 7, 2018 18:19
@bryevdv
Copy link
Member

bryevdv commented Sep 7, 2018

Thanks @Karel-van-de-Plassche !

xavArtley pushed a commit to xavArtley/bokeh that referenced this pull request Sep 7, 2018
* Added unit/integration tests for python3.7

* Remove python3.7 integration test
xavArtley pushed a commit to xavArtley/bokeh that referenced this pull request Sep 7, 2018
* Added unit/integration tests for python3.7

* Remove python3.7 integration test
xavArtley pushed a commit to xavArtley/bokeh that referenced this pull request Sep 7, 2018
* Added unit/integration tests for python3.7

* Remove python3.7 integration test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants