Update renderers and fix warning about AcceptValidHeader.best_match deprecation (C4-627)#1455
Open
netsettler wants to merge 15 commits intomasterfrom
Open
Update renderers and fix warning about AcceptValidHeader.best_match deprecation (C4-627)#1455netsettler wants to merge 15 commits intomasterfrom
netsettler wants to merge 15 commits intomasterfrom
Conversation
… 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).
willronchetti
approved these changes
Mar 15, 2021
Collaborator
Author
@willronchetti, as noted above, I plan to do a major version bump to highlight the shift in behavior when an |
Some triaging of old and new renderers.py and test_renderers.py.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 ofsrc/encoded/renderers.pygot ported, so that bears some scrutiny. All ofsrc/encoded/tests/test_renderers.pyis brought over. Note that doing just this much actually breaksfourfronttests because there was a misalignment in defaulting of mime types similar to something thatcgap-portalwent (harmlessly) through.In particular, if no mime-types are specified, this will default to
application/jsonnow, 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 anAcceptheader 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.pyis 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 thetesthtmlandanonhtmlfixtures bind theHTTP_ACCEPTenvironment variable totext/htmlrather than leaving it unbound (and effectively leaving to chance). It seems clear that these fixtures should expressly bindHTTP_ACCEPTto the content they want, and if anyone wants something different we could create fixturestest_anyandanon_anyor 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.pytosrc/encoded/tests/conftest_settings.pybecause it needs to be importable and one does not want to import the fixtures that are preloaded inpytest.ini. That seems to create some problem for PyTest. Doing this change meant several test files had to change to importORDERfromconftest_settings.pyinstead.Changed test_indexing to use
es_app_settingsinstead ofapp_settings. Because fo complicated reasons to do with the name-overriding of fixtures, I had to leave it calledapp, but that's whatcgap-portal. I think is probably all part of the original port Will did there.Opportunistic:
Makefilethese changes were made that I think are pretty non-controversial:psql-testand reimplementpsql-dev. These now work through new filescripts/psql-start.make kibana-start-testand reimplementmake kibana-start. These work through a revisedscripts/kibana-start.make test-performanceandmake testintegration`.make test-unitandmake test-npmso thatunitgoes first when doingmake teston a local machine for development. (This does not affect the parallel way that testing works on GitHub Actions.)make retest. I'm still not sure this works well enough to be of value, butpytestsays it's supposed to, so I want to hang onto it for a while as we do upgrades to newer versions ofpytestwhere it might work better. It would be really useful if it worked as advertised.