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

Update renderers and fix warning about AcceptValidHeader.best_match deprecation (C4-627) #1455

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

netsettler
Copy link
Collaborator

This ports various things from cgap-portal. In some cases, who files couldn't come over, so it's worth checking this to make sure the right parts did, but it makes the tests work. Please read the caveats in this bullet list to understand what needs most attention:

  • Port renderers support support and associated tests from cgap-portal. This means part of src/encoded/renderers.py got ported, so that bears some scrutiny. All of src/encoded/tests/test_renderers.py is brought over. Note that doing just this much actually breaks fourfront tests because there was a misalignment in defaulting of mime types similar to something that cgap-portal went (harmlessly) through.
    In particular, if no mime-types are specified, this will default to application/json now, but this was never formerly tested and I"m not convinced the old default of HTML was a good one. All clients should be offering an Accept header that will sort it out, and I think it's fair to say that modern browsers will send such a header.

    This is a small but important change. Whether it counts as a bug fix or a major version change is subjective. I'm inclined to think we should bump the major version just so if it results in issues it will be easy to spot. (Commit [6fa56b5](https://github.com/4dn-dcic/fourfront/commit/6fa56b5ac8cbe52bc05b57b632ece059dae8e6e9) is the relevant change.)

    Importantly, this will bring Fourfront and CGAP behavior back in line with one another. But also, we will have a set of unit tests that cover this space and if we need to make further adjustments we'll have a baseline to understand what we're affecting.

  • Much of src/encoded/tests/conftest.py is just rearranged into a better order and with some doc string added, but there is one change of substance made (in two places): the change that "fixes" the problem in renderers. That is, the "fix" is to the tests, to accept the new behavior of renderrs, not to the ported code, which makes the testhtml and anonhtml fixtures bind the HTTP_ACCEPT environment variable to text/html rather than leaving it unbound (and effectively leaving to chance). It seems clear that these fixtures should expressly bind HTTP_ACCEPT to the content they want, and if anyone wants something different we could create fixtures test_any and anon_any or something like that to test those cases as well, but we have never previously distinguished those cases. Probably we should add some renderers tests of those at some point.

  • Move ORDER definition from src/encoded/tests/datafixtures.py to src/encoded/tests/conftest_settings.py because it needs to be importable and one does not want to import the fixtures that are preloaded in pytest.ini. That seems to create some problem for PyTest. Doing this change meant several test files had to change to import ORDER from conftest_settings.py instead.

  • Changed test_indexing to use es_app_settings instead of app_settings. Because fo complicated reasons to do with the name-overriding of fixtures, I had to leave it called app, but that's what cgap-portal. I think is probably all part of the original port Will did there.

Opportunistic:

  • In Makefile these changes were made that I think are pretty non-controversial:
    • Implement psql-test and reimplement psql-dev. These now work through new file scripts/psql-start.
    • Implement make kibana-start-test and reimplement make kibana-start. These work through a revised scripts/kibana-start.
    • Implement make test-performance and make testintegration`.
    • Change the order of make test-unit and make test-npm so that unit goes first when doing make test on a local machine for development. (This does not affect the parallel way that testing works on GitHub Actions.)
    • Implement make retest. I'm still not sure this works well enough to be of value, but pytest says it's supposed to, so I want to hang onto it for a while as we do upgrades to newer versions of pytest where it might work better. It would be really useful if it worked as advertised.

… restart', 'make test-performance', and 'make test-integration'. Change order of 'make test-npm' and 'make test-unit' within 'make test' so that unit comes first.
…he rearranged fixture support from cgap-portal, which includes repairs to some of the html fixtures that specify they want text/html content. Move ORDER definition from datafixtures.py to conftest_settings.py. Adjust imports of ORDER. Mark test_fixtures as sloppy (causing this file not to run). Adjust test_indexing to use es_app/es_app_settings in places (as cgap does).
@netsettler netsettler changed the title Kmp c4 627 update renderers Update renderers and fix warning about AcceptValidHeader.best_match deprecation (C4-627) Mar 14, 2021
Copy link
Member

@willronchetti willronchetti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good to me, will need version bump when merged into master though (maybe target branch already has it)

@netsettler netsettler changed the base branch from kmp_fix_warnings_C4-629_C4-631 to master March 16, 2021 06:44
@netsettler
Copy link
Collaborator Author

will need version bump when merged into master

@willronchetti, as noted above, I plan to do a major version bump to highlight the shift in behavior when an Accept header isn't given. I fixed pyproject.toml to call this 3.0.0 and I've done a test deploy to fourfront-mastertest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants