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

Move fourfront from pytest 2.9 to 3.10 (ALT) #1437

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

Conversation

netsettler
Copy link
Collaborator

Substantive changes and bug fixes:

  • In src/encoded/authentication.py

    • From CGAP: Be explicit about encryption algorithms in JWT handling.
    • From CGAP: Remove special-case treatment of GET and HEAD methods.
  • pyproject.toml

    • Bump patch version.
    • Update pytest to version 3.10 and related libraries in some ways.
    • Update awscli, boto3, botocore, netaddr, boto3-stubs.
  • In src/encoded/schema_formats.py and src/encoded/server_defaults.py:

    • Use new ACCESSION_PREFIX variable to minimize number of places code diverges.
    • Add a TEST_PREFIX variable for symmetry. Compute the letter pairs needed for accession ids by looking at the schema files.
  • src/encoded/loadxl.py

    • From CGAP: Port the post_only= argument for load_all and load_all_gen.
  • src/encoded/renderers.py

    • From CGAP: Port mime type selection logic in should_transform.
    • From CGAP: Port best_mime_type.
  • src/encoded/tests/conftest.py

    • Change htmltestapp and anonhtmltestapp to pass HTTP_ACCEPT, which is more like what browsers would do, and which is needed for changes ported from CGAP.
    • Light PEP8.
    • Remove pytest_plugins setting, which is deprecated. This info is now in pytest.ini.
  • pytest.ini

    • Change to have markers in line with cgap-portal.
    • Specify testpaths rather than a --pyargs argument being added.
    • Use -p to handle plugins, in lieu of setting pytest_plugins.
  • src/encoded/tests/test_indexing.py

    • Change meta = MetaData(...); meta.reflect() to instead be meta = MetaData(..., reflect=True).
  • Makefile:

    • From CGAP: Port scripts/kibana-start and scripts/psql-start scripts to support targets kibana-start-test and psql-test, respectively.
    • Pin poetry 1.1.4
  • conftest.py

    • Set tempfile.tempdir to '/tmp'.
  • src/encoded/commands/purge_item_type.py

    • From CGAP: Change the way transaction commits work to be more incremental.
    • From CGAP: Use set(...) to remove duplicates.
  • Migrate the es_testapps to src/encoded/tests/conftest.py from src/encoded/tests/workbook.py,
    and update the following tests to use that test_app:

    • test_validation_errors.py
    • test_purge_item_type.py
    • test_static_page.py
    • test_indexing.py
    • test_aggregation.py
    • test_batch_download.py
  • Adjust Makefile target test to split indexing and non-indexing cases the way CGAP does.

Simple changes and small bug fixes (mostly found by better warnings):

  • Mark test_index_data_workbook as skip in test_search.py.

  • Use str in place of basestring in src/encoded/loadxl.py and src/encoded/tests/test_loadxl.py.

  • Port new file src/encoded/tests/test_renderers.py from CGAP.

  • src/encoded/tests/test_static_page.py

    • Changes to fixture scopes since session-scoped fixtures don't work well here.
  • src/encoded/types/page.py

    • Suppress deprecation warnings we aren't going to fix right now.
  • src/encoded/tests/test_types_biosample.py

    • Unmark biosample_relation as a fixture. It's just a function.
  • Add support for rewriting "~1.2.3" in setup_eb.py. Add test cases in test_setup_eb.py.

  • src/encoded/commands/spreadsheet_to_json.py

    • Remove an internal import of argparse.
  • src/encoded/xlreader.py

    • Add a missing module prefix (zipfile.ZipFile).
    • Light refactoring.
  • Relock poetry.lock.

  • Rename src/encoded/utils.py back to src/encoded/util.py for alignment with cgap-portal, and with history. Bad idea to have renamed it in the first place.

    • Port various utilities that might not be used, just in case they're needed for other ports. We can clean this up later.

    • Update callers to import from util:

      • src/encoded/types/file.py
      • src/encoded/tests/test_search.py
      • src/encoded/tests/test_types_init_collections.py
      • src/encoded/tests/test_types_protocol.py
      • src/encoded/tests/test_util.py (was test_utils.py, but renamed as well)
      • src/encoded/tests/test_validation_errors.py
      • src/encoded/tests/test_aggregation.py
      • src/encoded/tests/test_batch_download.py
  • Move the definition or ORDER to `conftest_setti

  • Get rid of unused config fixture on src/encoded/tests/conftest.py.

  • Get rid of unused moto fixture on src/encoded/tests/conftest.py.

Cosmetic changes:

  • Change .encrypt to .hash in

    • src/encoded/tests/test_access_key.py
    • src/encoded/types/access_key.py
    • src/encoded/tests/test_edw_hash.py
  • Use dcicutils.misc_utils.TestApp instead of webtest.TestApp in:

    • `src/encoded/commands/add_date_created.py
    • src/encoded/commands/import_data.py
    • src/encoded/commands/load_access_keys.py
  • .ebextensions/13_run_npm.config

    • Add a newline so this compares clean with CGAP.
  • .gitignore

    • From CGAP: Add various ignorable files.
  • src/encoded/commands/configure_kibana_index.py

    • Backport a TODO comment about {'use_ssl': True}.
  • src/encoded/dev_servers.py

    • Add PEP8 hint for PyCharm.
  • Also misc PEP8 in various files, including:

    • src/encoded/root.py

…tilities level. (Many are unused but may back other code we want to port.) Rename src/encoded/utils.py to src/encoded/util.py so these filenames will align again. Adjust files that import the renamed file. Also adjust test_indexing.py to use meta = MetaData(...); meta.reflect() instead of meta = MetaData(..., reflect=True).
…kibana-start-test and use poetry 1.1.4 in Makefile.
…ile.ZipFile) from cgap-portal version of scr/encoded/xlreader.py
… beause they'd be complicated to fix right now.
…iverges. Add a TEST_PREFIX variable for symmetry. Compute the letter pairs needed for accession ids by looking at the schema files.
…CCEPT, which is more like what browsers would do and is needed for some ported changes from cgap-portal. Light PEP8. Remove pytest_plugins setting, which is deprecated; this info is now in pytest.ini. Change pytest.ini to have markers in line with cgap-portal, to use testpaths rather than a --pyargs setting, and to use -p to handle plugins.
…nge the way commits work to be more incremental.
travis-test: # Actually, we don't normally use this. Instead the GA workflow sets up two parallel tests.
make travis-test-npm
make travis-test-unit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some of the changes in this file, and this PR in general, are just leveling with cgap-portal to minimize dissonance. But here we also added some criteria for what to use for make.

Note that in spite of all these markers, I still had to mark tests not to run with skip since otherwise if I try to run the file by name with, for example, bin/test -vv -k test_search it wouldn't confuse me by running things marked broken because I hadn't in my by-hand invocation listed all relevant markers.

The sloppy test is presently marking tests that break stuff because they don't manage side-effects well. It's possible that if they were divvied out differently, like with the indexing tests, it would be OK.

--pyargs encoded.tests deploy.tests
-p encoded.tests deploy.tests
-p encoded.tests.datafixtures
-p snovault.tests.serverfixtures
--instafail
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The above changes to addopts are what make pycharm work. It will invoke things with pytest, not bin/test and it's critical that all the options that are needed for it to run are correctly specified. In the old form, it worked to add a -k <something> on the command line to run a filtered test, but there was no way to run a single test because it was always going to run encoded.tests and deploy.tests no matter what you did. In the new way, you aren't forcing any tests to run by default. They are chosen as "all" if you specify no specific tests, and they're filtered against the directories specified later in the file (see testpaths below.

Pytest 3.10 also somewhere in there requires that you set the plugins information here on the command line (with -p) rather than inline in conftest.py files by setting pytest_plugins. It wants all that plugin stuff done at startup time, I think so that all the session fixtures go in those files and so it is known at startup which session things are needed to have session scope so it can activate them on first use reliably.

--pyargs encoded.tests deploy.tests
-p encoded.tests deploy.tests
-p encoded.tests.datafixtures
-p snovault.tests.serverfixtures
--instafail
markers =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I imported all the cgap marker names here. They don't create overhead and it makes it easier to copy code back and forth.

@@ -1,5 +1,60 @@
#!/bin/bash

docker_kibana_image=docker.elastic.co/kibana/kibana-oss:6.8.9
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file is just copied from cgap-portal. No code changes in this PR.

@@ -0,0 +1,70 @@
#!/bin/bash
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also just copied from cgap-portal without changes.

assert 'Accession and uuid are automatically assigned during initial posting' in res.json['content'][0]['content']
assert res.json['toc'] == posted_help_page['table-of-contents']


Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this test.

assert 'Accession and uuid are automatically assigned during initial posting' in res.json['content'][0]['content']
assert res.json['toc'] == posted_help_page['table-of-contents']


Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this test.



@pytest.fixture(scope='module')
def help_page_deleted(testapp, posted_help_page_section, help_page_json_draft):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that these fixtures were kind of a mess, with the draft json going into the deleted page, etc. I tried to clean that kind of thing up. It may be worth looking at the new file in direct mode, not side-by-side.

pytest.mark.schema,
pytest.mark.indexing,
#pytest.mark.flaky(rerun_filter=customized_delay_rerun(sleep_seconds=10))
]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: not an indexing test. It uses workbook.

def check_item_type(client, item_type):
# This might get a 404 if not enough time has elapsed, so try a few times before giving up.
return client.get('/%s?limit=all' % item_type, status=[200, 301]).follow()
class ItemTypeChecker:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is ported in from cgap-portal.

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.

I've added some small comments but the vast majority of these changes look reasonable to me and are unlikely to cause issues - I would consider deploying this somewhere to be sure prior to merging.

@@ -38,7 +40,7 @@ macpoetry-install: # Same as 'poetry install' except that on OSX Catalina, an e

configure: # does any pre-requisite installs
pip install --upgrade pip
pip install poetry==1.0.10 # pinned to avoid build problems we cannot fix in pyproject.toml
pip install poetry==1.1.4 # poetry latest as of 1/25/2021 seemed to work but apparantly does not
Copy link
Member

Choose a reason for hiding this comment

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

This comment is actually out of date now, 1.1.4 is latest and didn't work without the fix-dist-info step, which we now have so we're good.

Comment on lines +43 to +44
# TODO: CGAP does this differently. If port 443 is used, Will says use_ssl is implied and can be omitted here.
# Need to verify that and then make both systems agree here. -kmp 16-Feb-2021
Copy link
Member

Choose a reason for hiding this comment

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

Port 443 is specified by the server URL, so it is not needed here as well.

Comment on lines +113 to +114
"indexer": settings.get("indexer"),
"index_server": settings.get("index_server"),
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this have stayed the same?


pytestmark = [pytest.mark.indexing, pytest.mark.flaky(rerun_filter=delay_rerun)]
pytestmark = [pytest.mark.flaky(rerun_filter=delay_rerun), pytest.mark.triage]
Copy link
Member

Choose a reason for hiding this comment

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

I think the answer to this is "not working".

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