fourfront icon indicating copy to clipboard operation
fourfront copied to clipboard

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

Open netsettler opened this issue 3 years ago • 1 comments

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.

netsettler avatar Mar 14 '21 19:03 netsettler

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.

netsettler avatar Mar 16 '21 07:03 netsettler