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

Address warnings produced by tests #3032

Merged
merged 15 commits into from
Nov 30, 2022

Conversation

kevin-bates
Copy link
Member

This pull request takes a look at removing/ignoring a majority of the warnings that are produced by the server tests (via make test-server). Many of these warnings are ignored as they stem from upstream modules. That said, there are a number that have been resolved via code changes (primarily in tests) where things like file descriptors were not closed, etc.

We should strive to get to a place where all warnings (except those explicitly handled) are treated as errors. This will enable their capture in a timely manner so things like DeprecationWarning can be handled and adjusted (if necessary) or added to the list of ignored warnings (if appropriate).

This PR adds a number of warnings that are now ignored where, like caps on dependencies, a comment is provided indicating why the warning is ignored and/or additional context about the warning. At this point, the error filter is disabled. We could also consider ignoring these warnings using decorators in the test files themselves.

It looks like the number of warnings varies based on which version of Python is in use, but I'm seeing the warnings decrease from thousands of warnings to these 15:

================================================================================= warnings summary =================================================================================
elyra/tests/pipeline/test_pipeline_definition.py: 2 warnings
elyra/tests/pipeline/test_pipeline_parser.py: 1 warning
elyra/tests/pipeline/test_validation.py: 1 warning
elyra/tests/pipeline/airflow/test_component_parser_airflow.py: 9 warnings
elyra/tests/pipeline/airflow/test_processor_airflow.py: 2 warnings
  <unknown>:26: DeprecationWarning: invalid escape sequence '\_'

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================================================================ slowest durations =================================================================================

(1714 durations < 60s hidden.  Use -vv to show these durations.)
============================================================= 570 passed, 2 skipped, 15 warnings in 347.53s (0:05:47) ==============================================================

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@kevin-bates kevin-bates added area:back-end kind:CI Impacts continuous integration (build, test, etc.) labels Nov 29, 2022
@elyra-bot
Copy link

elyra-bot bot commented Nov 29, 2022

Thanks for making a pull request to Elyra!

To try out this branch on binder, follow this link: Binder

@kevin-bates
Copy link
Member Author

Moving to draft to try to address remaining warnings.

@kevin-bates kevin-bates marked this pull request as draft November 29, 2022 16:43
@ptitzler ptitzler added this to the 3.14.0 milestone Nov 29, 2022
@kevin-bates kevin-bates marked this pull request as ready for review November 29, 2022 19:05
@kevin-bates
Copy link
Member Author

The last 15 warnings will go away once this PR is merged. The reason is that one of the test resources is referenced within the main GitHub branch within the tests, so those changes won't be available until after the merge. (I temporarily pointed at my branch to confirm.)

I also fixed the test-artifact "leak" where the test_processor_kfp.py tests were producing these files in the dev area:

	elyra/tests/pipeline/resources/test_pipelines/kfp/kfp-one-node-generic-elyra-properties.yaml
	elyra/tests/pipeline/resources/test_pipelines/kfp/kfp-one-node-generic.yaml

This ultimately interferes with the dev's ability to check in changes w/o first removing those files or being explicit on each commit.

And removed the use of the requests.session() function, which was deprecated, and replaced it with requests.sessions.Session().

Copy link
Member

@ptitzler ptitzler left a comment

Choose a reason for hiding this comment

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

LGTM!

@ptitzler ptitzler added the component:test Test-related label Nov 29, 2022
Signed-off-by: Kevin Bates <kbates4@gmail.com>
Signed-off-by: Kevin Bates <kbates4@gmail.com>
Signed-off-by: Kevin Bates <kbates4@gmail.com>
Signed-off-by: Kevin Bates <kbates4@gmail.com>
Signed-off-by: Kevin Bates <kbates4@gmail.com>
Signed-off-by: Kevin Bates <kbates4@gmail.com>
Signed-off-by: Kevin Bates <kbates4@gmail.com>
Signed-off-by: Kevin Bates <kbates4@gmail.com>
Signed-off-by: Kevin Bates <kbates4@gmail.com>
Signed-off-by: Kevin Bates <kbates4@gmail.com>
Signed-off-by: Kevin Bates <kbates4@gmail.com>
Signed-off-by: Kevin Bates <kbates4@gmail.com>
Signed-off-by: Kevin Bates <kbates4@gmail.com>
Signed-off-by: Kevin Bates <kbates4@gmail.com>
Signed-off-by: Kevin Bates <kbates4@gmail.com>
@akchinSTC akchinSTC merged commit 695321b into elyra-ai:main Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:back-end component:test Test-related kind:CI Impacts continuous integration (build, test, etc.) sizing: XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants