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

Setup python paths in test runner configuration #20832

Merged
merged 6 commits into from
Sep 1, 2020

Conversation

jsoriano
Copy link
Member

Add conftest.py files to projects with python tests. This file is imported by the test runner. Use it to setup python paths so tests don't need to care about them depending on where they are located, and we control paths from central points.

Use the python tests runner to setup python paths so each test doesn't
need to setup their own python paths.
@jsoriano jsoriano added needs_backport PR is waiting to be backported to other branches. :Testing Team:Integrations Label for the Integrations team v7.10.0 labels Aug 27, 2020
@jsoriano jsoriano requested review from a team as code owners August 27, 2020 16:34
@jsoriano jsoriano self-assigned this Aug 27, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Aug 27, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Aug 27, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #20832 updated]

  • Start Time: 2020-08-27T21:36:32.851+0000

  • Duration: 65 min 59 sec

Test stats 🧪

Test Results
Failed 0
Passed 15289
Skipped 1761
Total 17050

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

I would prefer to have the import statements match the project structure so that when you see an import statement you can know exactly where the file is coming from (like import libbeat.tests.system.beat.compose). Then the only addition to sys.path is the repository root. Would it be possible to implement that? Would you still need one conftest.py for every beat project?

@jsoriano
Copy link
Member Author

jsoriano commented Aug 27, 2020

I would prefer to have the import statements match the project structure so that when you see an import statement you can know exactly where the file is coming from (like import libbeat.tests.system.beat.compose). Then the only addition to sys.path is the repository root. Would it be possible to implement that? Would you still need one conftest.py for every beat project?

It is not very common to have so long import paths in python. Usually the modules are in some path, and one adds this path to their python path.
An extra problem is that all the intermediary paths need to be modules too, so for libbeat.test.system.beat.compose to work, we would need to add some empty __init__.py files:

libbeat/__init__.py
libbeat/tests/__init__.py
libbeat/tests/system/__init__.py

Something we could do if we want to have a single conftest.py file instead of one per project, is to move all python modules we have to some common path (dev-tools/python?), and include this path from a single conftest.py in the root of the project. We would need to slightly refactor packages used in x-pack. I will give a try to this option. @andrewkroh wdyt about this option?

@jsoriano
Copy link
Member Author

I would prefer to have the import statements match the project structure so that when you see an import statement you can know exactly where the file is coming from.

Another problem with this is that we have python code under directories with hyphens (x-pack), that are not allowed in python module names.

In [9]: import x-pack
  File "<ipython-input-9-1b6cd5801f44>", line 1
    import x-pack
            ^
SyntaxError: invalid syntax

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

Thanks for explaining the reasons why my suggestion would not work. I'm not much of a python developer (nor do I want to be 😆 ). I think this approach of moving the sys.path manipulation out of the code is clean.

@jsoriano
Copy link
Member Author

jsoriano commented Sep 1, 2020

I am merging this as it removes python path configuration from tests, what I think is an improvement.
As a future improvement, we could still look for ways of not needing a conftest.py file per project.

@jsoriano jsoriano merged commit 42a53eb into elastic:master Sep 1, 2020
@jsoriano jsoriano deleted the python-tests-paths branch September 1, 2020 11:29
@jsoriano jsoriano removed the needs_backport PR is waiting to be backported to other branches. label Sep 1, 2020
jsoriano added a commit to jsoriano/beats that referenced this pull request Sep 1, 2020
Use the python tests runner to setup python paths so each test doesn't
need to setup their own python paths.

(cherry picked from commit 42a53eb)
jsoriano added a commit that referenced this pull request Sep 1, 2020
Use the python tests runner to setup python paths so each test doesn't
need to setup their own python paths.

(cherry picked from commit 42a53eb)
v1v added a commit to v1v/beats that referenced this pull request Sep 2, 2020
…ne-2.0

* upstream/master: (87 commits)
  [packaging] Normalise GCP bucket folder structure (elastic#20903)
  [Metricbeat] Add billing metricset into googlecloud module (elastic#20812)
  Include python docs in devguide index (elastic#20917)
  Avoid generating incomplete configurations in autodiscover (elastic#20898)
  Improve docs of leaderelection configuration (elastic#20601)
  Document how to set the ES host and Kibana URLs in Ingest Manager (elastic#20874)
  docs: Update beats for APM (elastic#20881)
  Adding cborbeat to community beats (elastic#20884)
  Bump kibana version to 7.9.0 in x-pack/metricbeat (elastic#20899)
  Kubernetes state_daemonset metricset for Metricbeat (elastic#20649)
  [Filebeat][zeek] Add new x509 fields to zeek (elastic#20867)
  [Filebeat][Gsuite] Add note about admin in gsuite docs (elastic#20855)
  Ensure kind cluster has RFC1123 compliant name (elastic#20627)
  Setup python paths in test runner configuration (elastic#20832)
  docs: Add  `processor.event` info to Logstash output (elastic#20721)
  docs: update cipher suites (elastic#20697)
  [ECS] Update ecs to 1.6.0 (elastic#20792)
  Fix path in hits docs (elastic#20447)
  Update filebeat azure module documentation (elastic#20815)
  Remove duplicate ListGroupsForUsers in aws/cloudtrail (elastic#20788)
  ...
melchiormoulin pushed a commit to melchiormoulin/beats that referenced this pull request Oct 14, 2020
Use the python tests runner to setup python paths so each test doesn't
need to setup their own python paths.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Integrations Label for the Integrations team :Testing v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants